feat(Taxonomy): allow fetching taxonomies from other flavors (obf, opff, opf)#466
feat(Taxonomy): allow fetching taxonomies from other flavors (obf, opff, opf)#466
Conversation
| """ | ||
| taxonomy_type = TaxonomyType[taxonomy_type] | ||
| filename = f"{taxonomy_type.name}.json" | ||
| filename = f"{flavor.name}-{taxonomy_type.name}.json" |
There was a problem hiding this comment.
not necessarily needed, but avoid conflicts if the user has in its cache the same taxonomy from different flavors
|
|
||
| def get_taxonomy( | ||
| taxonomy_type: TaxonomyType | str, | ||
| flavor: Flavor = Flavor.off, |
There was a problem hiding this comment.
kinda frustrating that get_taxonomy() has type THEN flavor
whereas get_dataset() has flavor THEN type ^^
There was a problem hiding this comment.
Maybe, though we’d want flavor to default to OFF anyway, so it was never going to be the first parameter, since we do not want to set a default taxonomy_type.
Also, when invoking the function, you can always invoke with keyword-only arguments, and then you get to decide the order yourself. ;)
There was a problem hiding this comment.
yes but
- the taxonomy_type does not have a default, so it has to be passed as an argument
- and in python you can't have defaulted parameters before non-defaulted ones
so we're stuck ^^ - though you're saying that when invoking with parameters i can reorder them ? i need to try 👀
but in the end, re-reading this, well dataset_type & taxonomy_type are not really comparable in value, just that their name is similar haha
Freso
left a comment
There was a problem hiding this comment.
With all the changes here, and esp. with all the repetition, this might be better to make into a function? Maybe something like:
def get_taxonomy_url(taxonomy_type, flavor=off) -> str:
if flavor = off:
valid_taxonomies = (categories, …, other_nutritional_substances)
# these sets of valid taxonomies should maybe be
# new globals in place of TAXONOMY_URLS, in case
# other code wants to reference it?
elif flavor = obf:
valid_taxonomies = (categories, …, allergens)
elif …:
…
else:
# Unrecognised flavor
raise UnrecognisedFlavorError
# or return None?
if taxonomy_type in valid_taxonomies:
return URLBuilder.static(flavor, Environment.org)
+ f"/data/taxonomies/{taxonomy_type}.full.json",
else:
raise UnknownTaxonomyTypeError
# or return None?Or subclass URLBuilder into a TaxonomyURLBuilder class, overriding the static() method to take flavor, taxonomy_type?
|
|
||
| def get_taxonomy( | ||
| taxonomy_type: TaxonomyType | str, | ||
| flavor: Flavor = Flavor.off, |
There was a problem hiding this comment.
Maybe, though we’d want flavor to default to OFF anyway, so it was never going to be the first parameter, since we do not want to set a default taxonomy_type.
Also, when invoking the function, you can always invoke with keyword-only arguments, and then you get to decide the order yourself. ;)
|
| ) | ||
| + "/data/taxonomies/other_nutritional_substances.full.json", | ||
| Flavor.off: { | ||
| TaxonomyType.category: URLBuilder.static(Flavor.off, Environment.org) |
There was a problem hiding this comment.
As @Freso was suggesting, I also think it would be worth creating a get_taxonomy_url function that would take as argument the flavor and the filename, to shorten a bit this dict.
There was a problem hiding this comment.
I'll do this in a followup PR 👍



Description
Similar to #223, extend the
get_taxonomylogic to allow fetching taxonomies for non-off favors.Solution
get_taxonomy()logicflavorparam, defaults tooffTAXONOMY_URLSdictbody_parts.full.jsonbut this can be done in a another PR laterRelated issue(s)