[DO] Use methods instead of properties for discovery #979

esloho · 2019-09-12T15:26:10Z

Closes #961

Besides the conversion of properties into methods, this PR also includes:

Raising an error if no entity is found in the server for a given id (instead of returning None)
Renaming of entities and repositories methods for a more Python-like flavor (less verbose)
Use of an internal instance for every Repository class to be used commonly instead of creating a new instance for each single use
Addition of a new notebook to be used as example for the discovery part (in examples/07_data_observatory/discovery.ipynb ). Still a work in progress, but it's a start (and useful for local testing while developing).

Fix conflicts

test/data/observatory/test_provider.py

test/data/observatory/repository/test_variable_repo.py

test/data/observatory/repository/test_provider_repo.py

test/data/observatory/repository/test_geography_repo.py

test/data/observatory/repository/test_dataset_repo.py

test/data/observatory/repository/test_country_repo.py

test/data/observatory/repository/test_category_repo.py

test/data/observatory/examples.py

alrocar

Super clean implementation! ❤️

Left some minor comments about the messaging and some others to agree on the API design.

alrocar · 2019-09-13T09:03:48Z

cartoframes/data/observatory/category.py

@@ -18,12 +18,11 @@ def _constructor_expanddim(self):
        return Categories

    @staticmethod
-    def get_by_id(category_id):
-        return get_category_repo().get_by_id(category_id)
+    def by_id(category_id):


I guess this is just syntax sugar but let's agree on this API design decision.

I liked the get_* version for a couple of reasons:

In general I see functions as verbs (where reasonable)

You have all the get functions under the same umbrella. It's easier if you use an auto-complete system to write get and see what different options you have to get Categories

There are other frameworks (such as Django) that follow that convention get_by_*

The case for countries() and categories() is a bit different, those are known classes and since they can't just be properties I'm OK with keeping them as nouns

Thoughts? (not sure if we changed it for any other reason)

I like those reasons. I changed back all the methods names 👍

alrocar · 2019-09-13T09:07:39Z

cartoframes/data/observatory/repository/category_repo.py

        result = self.client.get_categories('id', category_id)

        if len(result) == 0:
-            return None
+            raise CartoException('The id does not correspond with any existing category in Data Observatory.')


Adding a comment here to take into account in the rest of classes

I'd use a subclass of CartoException for this, something like ObjectDoesNotExistError or similar. So we can let the user to just catch CartoException or doing a more fine grained exception handling.

Also, I'd replace in Data Observatory by in the catalog. I'd try to add some hint on what to do next, like "you can list the available list of categories IDs by calling to Category.all()" (or whatever)

Should this be an extension of CartoException (which I see is defined in the CARTO's boilerplate) or an unrelated one?

mmm I don't have a strong opinion about that, maybe we can have a generic DiscoveryException or similar, unrelated from CartoException which tend to be more related to server side errors.

What do you think?

I created a dedicated exception DiscoveryException in a new module exceptions at the root package. If we create new ones in the future then we could create a package exceptions with modules for each group. Does it sounds OK?

alrocar · 2019-09-13T09:18:36Z

examples/07_data_observatory/discovery.ipynb

+   "source": [
+    "# Discoverying content of interest in the Data Observatory\n",
+    "\n",
+    "CARTO's Data Observatory is a powerful tool for exploring the available datasets in our data lake. Through the discovery API you would be able to navigate through the datasets and their properties, thus knowing in advanced which sources may be of interest for you before even requesting access to them. "


I'd write this as:

Suggested change

"CARTO's Data Observatory is a powerful tool for exploring the available datasets in our data lake. Through the discovery API you would be able to navigate through the datasets and their properties, thus knowing in advanced which sources may be of interest for you before even requesting access to them. "

"The Discovery API is a powerful tool for exploring the available datasets in our Data Observatory catalog. Through the Discovery API you would be able to navigate through the datasets and their variables, thus knowing in advanced which sources may be of interest for you before even requesting access to them. "

Done! I changed all the mentions in the notebook.

alrocar · 2019-09-13T09:23:49Z

examples/07_data_observatory/discovery.ipynb

@@ -0,0 +1,253 @@
+{


Let's move this to a 07_discovery or 07_catalog folder

alrocar · 2019-09-13T11:38:57Z

cartoframes/data/observatory/provider.py

@@ -0,0 +1,56 @@
+import pandas as pd
+
+from cartoframes.data.observatory.repository.provider_repo import get_provider_repo


just realized according to our convention we forgot to use relative imports in this PR.

alrocar · 2019-09-13T11:48:19Z

examples/07_catalog/discovery.ipynb

@@ -0,0 +1,253 @@
+{


Just ran the notebook and it seems it's outdated (still using properties for countries or datasets or variables. Please try to run it locally to fix those errors.

alrocar · 2019-09-13T11:51:37Z

cartoframes/data/observatory/repository/category_repo.py

        result = self.client.get_categories('id', category_id)

        if len(result) == 0:
-            return None
+            raise CartoException('The id does not correspond with any existing category in Data Observatory.')


mmm I don't have a strong opinion about that, maybe we can have a generic DiscoveryException or similar, unrelated from CartoException which tend to be more related to server side errors.

What do you think?

alrocar

Let's re-run and fix the notebook, seems it got outdated.

🏓

alrocar

VAMOS!! 🚀

esloho added 6 commits September 11, 2019 13:39

Update branch with changes from previous PR

b31e10f

Fix conflicts

Rename repo methods and extract instance to a variable

0441607

Rename entities methods

a1253cc

Add missing tests

c50498a

Raise error if element not found in server by id

2cb5067

Add notebook with examples for discovery

09cebd5

houndci-bot reviewed Sep 12, 2019

View reviewed changes

esloho requested review from simon-contreras-deel and alrocar September 12, 2019 15:30

Fix missing final blank line

750bcb4

alrocar added the discovery label Sep 13, 2019

alrocar reviewed Sep 13, 2019

View reviewed changes

alrocar assigned esloho Sep 13, 2019

esloho added 2 commits September 13, 2019 12:39

Rename methods to bring back the get_ prefix

5ebc1a5

Rename example of data catalog and rephrase some text

c9812c6

esloho assigned alrocar and unassigned esloho Sep 13, 2019

alrocar reviewed Sep 13, 2019

View reviewed changes

alrocar suggested changes Sep 13, 2019

View reviewed changes

alrocar assigned esloho and unassigned alrocar Sep 13, 2019

Update notebook and create dedicated exception

8fa5b8e

esloho assigned alrocar and unassigned esloho Sep 16, 2019

esloho added 4 commits September 16, 2019 12:59

Fix exceptions access from tests

f9d804a

Add simple test to confirm Travis-error theory

c3302de

Kids remember to add the new files before commiting

2baf954

Fix repo key file

80e7930

esloho requested a review from alrocar September 16, 2019 12:36

alrocar approved these changes Sep 16, 2019

View reviewed changes

alrocar removed their assignment Sep 16, 2019

alrocar merged commit 18f0be2 into develop Sep 16, 2019

alrocar deleted the feature/961-properties branch September 16, 2019 13:41

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[DO] Use methods instead of properties for discovery #979

[DO] Use methods instead of properties for discovery #979

esloho commented Sep 12, 2019

alrocar left a comment

alrocar Sep 13, 2019

esloho Sep 13, 2019

alrocar Sep 13, 2019

esloho Sep 13, 2019

alrocar Sep 13, 2019

esloho Sep 16, 2019

alrocar Sep 13, 2019

esloho Sep 13, 2019

alrocar Sep 13, 2019

esloho Sep 13, 2019

alrocar Sep 13, 2019

alrocar Sep 13, 2019

alrocar Sep 13, 2019

alrocar left a comment •

edited

Loading

alrocar left a comment

	"CARTO's Data Observatory is a powerful tool for exploring the available datasets in our data lake. Through the discovery API you would be able to navigate through the datasets and their properties, thus knowing in advanced which sources may be of interest for you before even requesting access to them. "
	"The Discovery API is a powerful tool for exploring the available datasets in our Data Observatory catalog. Through the Discovery API you would be able to navigate through the datasets and their variables, thus knowing in advanced which sources may be of interest for you before even requesting access to them. "

		@@ -0,0 +1,56 @@
		import pandas as pd

		from cartoframes.data.observatory.repository.provider_repo import get_provider_repo

[DO] Use methods instead of properties for discovery #979

[DO] Use methods instead of properties for discovery #979

Conversation

esloho commented Sep 12, 2019

alrocar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alrocar left a comment • edited Loading

Choose a reason for hiding this comment

alrocar left a comment

Choose a reason for hiding this comment

alrocar left a comment •

edited

Loading