DO info #1311
DO info #1311
Conversation
simon-contreras-deel
requested review from
Jesus89
and removed request for
Jesus89
fix do open data and first subscription
… v4-me-endpoint
|
Not sure why tests are failing, @oleurud should I start reviewing or do you prefer having 🍏 tests? |
|
Although it is falling in travis, we can do the review and the acceptance from a local version. Of course, before merging, we need to realease carto-python |
| @@ -497,3 +498,6 @@ def _get_summary_data(self): | |||
| else: | |||
| log.info('Summary information is not available') | |||
| return None | |||
|
|
|||
| def __str__(self): | |||
| return "<Dataset.get('{}')>".format(self._get_print_id()) | |||
There was a problem hiding this comment.
It is a @arredond request. When an error happens, we are showing a very huge error message due to the big string created for the Dataset and basically it is difficult to understand
| @@ -147,9 +150,21 @@ def _get_credentials(self, credentials=None): | |||
|
|
|||
| return _credentials | |||
|
|
|||
| def _get_remote_full_table_name(self, user_project, user_dataset, public_project): | |||
There was a problem hiding this comment.
It's weird having this method and the _download one in Entity. I'd suggest to refactor to a DownloadableEntity class and if we want to keep using inheritance, then make it to extend Entity and Dataset and Geography inherit from that DownloadableEntity.
If we are not going to do it now, please, let's create a separate issue so it can be prioritized at some point.
There was a problem hiding this comment.
_get_remote_full_table_name is useful in any case because we are basically getting this info in different places but IMO the entity is responsible for that. I will do the same for Variable in the future if everybody agrees.
About _download method is a private one and it is called from Catalog and Geography entities. For me, a DownloadableEntity makes sense but don't you think we already have a lot of classes in the catalog?
|
|
||
| try: | ||
| file_path = bq_client.download_to_file(_WORKING_PROJECT, user_dataset, view) | ||
| file_path = bq_client.download_to_file(project, dataset, table, file_path) |
There was a problem hiding this comment.
There are still some warns below these lines, could we use a log.info instead?
| @@ -147,9 +150,21 @@ def _get_credentials(self, credentials=None): | |||
|
|
|||
| return _credentials | |||
|
|
|||
| def _get_remote_full_table_name(self, user_project, user_dataset, public_project): | |||
| project, dataset, table = self.id.split('.') | |||
There was a problem hiding this comment.
I see we do the split thing in several places to take the project, dataset and table_name. I'd wrap that into a utils method or something.
There was a problem hiding this comment.
It is a bit weird, because you must know the order when doing the split. Probably, these entities (at least Dataset, Geography, and Variable) should have project, dataset and table_name as properties to avoid doing that. I would tackle it in a different PR
Fix:
Note: it needs a new version of
carto-python: CartoDB/carto-python#155Acceptance:
Step 1: using user without any dataset or geography subscribed:
Step 2:
Step 3: