Conversation
JoachimZ
left a comment
JoachimZ
left a comment
There was a problem hiding this comment.
Since it's a plugin that is supposed to work now, please check hello world works in py2 and 3 (and write the you already tested that in the PR desc)
| logger.error("Setting schema") | ||
| input_schema_names = frozenset([e['name'] for e in input.read_schema()]) | ||
| output_schema = input.read_schema() | ||
| print(')))))))))') |
| from dataiku.customrecipe import * | ||
| import logging | ||
|
|
||
| logger = logging.getLogger(__name__) |
There was a problem hiding this comment.
Isn't basic config necessary?
I'd rather not use name and not even use logger =
just use logging.basicConfig with a decent format (for example I probably did something not horrible for example in azure cognitive services plugins) and then just use logging.info
There was a problem hiding this comment.
+1 to Joachim, we should share config (nothing fancy, just simple stuff) across plugins)
| elif self.config['api_url'].startswith('https://extraction.import.io/'): | ||
| self.api_version = 'extraction' | ||
| else: | ||
| raise Exception( |
There was a problem hiding this comment.
maybe more specifically type the Exception, not necessarily custom but just a more descriptive exception class
There was a problem hiding this comment.
also we should probably log the exception proper (might be having a moment - does raise do that natively?)
| raise Exception( | ||
| 'It looks like this URL is not an API URL. URLs to call the API (and get a json response) start with "https://api.import.io" .') | ||
| print '[import.io connector] calling API...' | ||
| raise Exception('It looks like this URL is not an API URL. URLs to call the API (and get a json response) start with "https://api.import.io" .') |
There was a problem hiding this comment.
it seems like from the above if/else the url can also start with another prefix? Might be confusing to not log that
| elif self.config['api_url'].startswith('https://extraction.import.io/'): | ||
| self.api_version = 'extraction' | ||
| else: | ||
| raise Exception( |
There was a problem hiding this comment.
also we should probably log the exception proper (might be having a moment - does raise do that natively?)
| raise ValueError | ||
|
|
||
| def get_read_schema(self): | ||
| if self.api_version == 'api': |
There was a problem hiding this comment.
nit: couldn't api-version just be a None like value?
| return None | ||
|
|
||
| def generate_rows(self, dataset_schema=None, dataset_partitioning=None, partition_id=None, records_limit = -1): | ||
| if self.api_version == 'api': |
There was a problem hiding this comment.
why are we testing this in every function? also why would api version be api
| from dataiku.customrecipe import * | ||
| import logging | ||
|
|
||
| logger = logging.getLogger(__name__) |
There was a problem hiding this comment.
+1 to Joachim, we should share config (nothing fancy, just simple stuff) across plugins)
| logger.error('request to import.io failed') | ||
| logger.error(e) | ||
| logger.error('response was: {}'.format(response)) | ||
| raise ValueError |
There was a problem hiding this comment.
aren't we masking exceptions? Why not just throw the actual exception? (There's sometimes reason to do this but I don't know if that is the case here as a number of things could be going wrong and it might help user to know)
| logger.error("Setting schema") | ||
| input_schema_names = frozenset([e['name'] for e in input.read_schema()]) | ||
| output_schema = input.read_schema() | ||
| print(')))))))))') |
There was a problem hiding this comment.
+1 with Joachim, idk what is going on here
2 participants
DO NOT MERGE