Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Country.categories and Category.geographies properties #1093

Merged
merged 11 commits into from
Oct 14, 2019

Conversation

esloho
Copy link
Contributor

@esloho esloho commented Oct 11, 2019

Closes #1038

I added the missing methods and then I realized we could always use get_all for those properties so I did a bit of cleaning in code and tests ✨

@esloho esloho mentioned this pull request Oct 11, 2019
Copy link
Contributor

@alrocar alrocar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@alrocar
Copy link
Contributor

alrocar commented Oct 11, 2019

Acceptance 🍊

I've tested the whole list of methods in the original issue and I found a couple of missing ones. Let me know if we should add them:

  • Category.get('demographics').countries
  • Not sure about this, but properties that are actually a FK are returned as an ID instead of the class instance. Examples (not a comprehensive list I guess). I'd say this is not a big deal and in any case I'd open a separate issue in case we want to support that:
    • Dataset.get('od_acs_1f614ee8').geography
    • Variable.get('poverty_b698f5ff').dataset
    • Geography.get('carto-do-public-data.tiger.geography_usa_countyclipped_2015').country

@alasarr
Copy link
Contributor

alasarr commented Oct 13, 2019

Hi, it'd be great if get method for CatalogList should work with slug too.

Dataset.get(<dataset_id>).variables.get().

If we do that we can say that at everypart we can find by slug or id.

@esloho
Copy link
Contributor Author

esloho commented Oct 14, 2019

  • Category.get('demographics').countries

A category has no countries nor direct relation with them, so I understood that was a nested filter use case. You should do:

catalog.category('demographics').countries

However, if we want to have the first option as well, I'll add it. (I already talked to Aroa last week about the fact that we seem to always have 2 ways of doing things, and it's not good.)

  • Not sure about this, but properties that are actually a FK are returned as an ID instead of the class instance. Examples (not a comprehensive list I guess). I'd say this is not a big deal and in any case I'd open a separate issue in case we want to support that:

    • Dataset.get('od_acs_1f614ee8').geography
    • Variable.get('poverty_b698f5ff').dataset
    • Geography.get('carto-do-public-data.tiger.geography_usa_countyclipped_2015').country

We discussed this a couple of weeks ago and decided (at least for the moment) to have all FK properties returning the id (but rename them to geography instead of geography_id in the API). The idea was to have it return the whole objects eventually. Should we address this now?

@alrocar
Copy link
Contributor

alrocar commented Oct 14, 2019

We discussed this a couple of weeks ago and decided (at least for the moment) to have all FK properties returning the id (but rename them to geography instead of geography_id in the API). The idea was to have it return the whole objects eventually. Should we address this now?

Sorry, it's just I was running the full acceptance cycle and I wasn't aware we decided that 😊

However, if we want to have the first option as well, I'll add it. (I already talked to Aroa last week about the fact that we seem to always have 2 ways of doing things, and it's not good.)

Let's do that validation work with @newLadyAga

@esloho are you going to address this in this PR or in a different one?

@esloho
Copy link
Contributor Author

esloho commented Oct 14, 2019

Hi, it'd be great if get method for CatalogList should work with slug too.

Done @alasarr !!

@esloho esloho mentioned this pull request Oct 14, 2019
@alasarr
Copy link
Contributor

alasarr commented Oct 14, 2019

Working! and added to the demo!

@esloho esloho removed their assignment Oct 14, 2019
@alrocar alrocar merged commit c755e7d into develop Oct 14, 2019
@alrocar alrocar deleted the feature/1038-add-missing-methods branch October 14, 2019 14:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Catalog by smart classes
4 participants