-
-
Notifications
You must be signed in to change notification settings - Fork 407
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
feat: Extract ingredients origins from labels and use them in Eco-Score #6377
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, eventual cosmetic changes possibles
lib/ProductOpener/Ingredients.pm
Outdated
sub has_specific_ingredient_property($$$) { | ||
|
||
my $product_ref = shift; | ||
my $ingredient_id = shift; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would rename as searched_id
because it's very hard not to mess things up underneath between ingredient_id ingredient_ref and specific_ingredient_id.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
otherwise, underneath use specific rather than ingredient in name: specific_ref
, specific_id
lib/ProductOpener/Ingredients.pm
Outdated
and ((not defined $ingredient_id) or (is_a("ingredients", $ingredient_id, $specific_ingredient_id)))) { | ||
|
||
$value = $ingredient_ref->{origins}; | ||
last; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm asking myself in if this kind of treatment we should not continue for sanity checks. Something like:
defined $value and $value = $ingredient_ref->{origins}
or log->warning("Got more than one specific ingredient origin ", {product => $product_ref->{id}, ingredient_id => $ingredient_id});
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good idea. I also just noticed that I hardcoded "origins" in this function even though it's passed a $property parameter.
en:French pork | ||
fr:Viande Porcine Française, VPF, viande de porc française, Le Porc Français, Porc Origine France, porc français, porc 100% France | ||
origins:en: en:france | ||
ingredients:en: en:pork |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should document that origins can be a list, but not ingredients (from what I understand).
Co-authored-by: Alex Garel <alex@garel.org>
Co-authored-by: Alex Garel <alex@garel.org>
Kudos, SonarCloud Quality Gate passed! |
What
3 changes:
en:French eggs, Eggs from France
ingredients:en: en:egg
origins:en: en:france
This info is added to the "specific_ingredients" structure, which can also be populated through parsing the end of the ingredient list (e.g. phrases like "Origin of the milk: United Kingdom").
The origins of specific ingredients are now used during ingredients analysis to populate the origins field of matching ingredients.
For the Eco-Score, if we don't have an ingredients list (e.g. products that have a single ingredient like eggs often don't have an ingredient list filled), but if we have an ingredient specific origin (French eggs), we use that origin for the whole product.
Part of