-
-
Notifications
You must be signed in to change notification settings - Fork 404
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
fix: new additives, non-nutritive sweeteners for new Nutri-Score #9005
Conversation
…/nuts or legumes for Nutri-Score
Codecov Report
@@ Coverage Diff @@
## main #9005 +/- ##
==========================================
+ Coverage 46.12% 46.25% +0.13%
==========================================
Files 64 64
Lines 19850 19883 +33
Branches 4795 4803 +8
==========================================
+ Hits 9155 9197 +42
+ Misses 9528 9522 -6
+ Partials 1167 1164 -3
... and 4 files with indirect coverage changes 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
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.
Great. Some comments on minor problems
lib/ProductOpener/Ingredients.pm
Outdated
my %non_nutritive_sweeteners = ( | ||
"en:e950" => 1, | ||
"en:e951" => 1, | ||
"en:e952" => 1, | ||
"en:e954" => 1, | ||
"en:e955" => 1, | ||
"en:e957" => 1, | ||
"en:e959" => 1, | ||
"en:e960" => | ||
1, # E960 is not listed in the Nutri-Score table as it was replaced by E960a/b/c/d, we assume it's E960a | ||
"en:e960a" => 1, | ||
"en:e961" => 1, | ||
"en:e962" => 1, | ||
"en:e969" => 1, | ||
); |
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 find a bit odd to have that kind of data directly in the code ! (but I don't have a good cheap alternative).
Apart maybe from putting it in Config.pm ?
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.
An alternative approach would be to use a property in the additives taxonomy.
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.
Removed the list of additives and created a new non_nutritive_sweeteners property in additives.txt instead.
lib/ProductOpener/Ingredients.pm
Outdated
foreach my $additive (@{$product_ref->{'additives_tags'}}) { | ||
if (exists $non_nutritive_sweeteners{$additive}) { | ||
$product_ref->{with_non_nutritive_sweeteners} = 1; | ||
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 think having a search_list and intersect_list methods for this kind of thing that comes over and over would be great
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.
@alexgarel I added generic functions in Tags.pm:
get_property_from_tags
get_inherited_property_from_tags
get_matching_regexp_property_from_tags
elsif (defined $ingredient_ref->{percent_estimate}) { | ||
$count += $ingredient_ref->{percent_estimate}; |
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.
did you check it's not referenced in OpenAPI (if it is, change it there also, and add a description if there is none yet !)
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.
in the doc we just said "estimate", not "minimum estimate", so the doc is correct.
I added the new "fruits-vegetables-legumes-estimate-from-ingredients" field.
tests/integration/expected_test_results/api_v2_product_read/get-existing-product.json
Show resolved
Hide resolved
tests/integration/expected_test_results/export_more_fields/rows/7804659650035.json
Show resolved
Hide resolved
...xpected_test_results/search_v1/search-tags-categories-without-ingredients-from-palm-oil.json
Show resolved
Hide resolved
@@ -469,6 +520,7 @@ foreach my $test_ref (@tests) { | |||
compute_serving_size_data($product_ref); | |||
compute_field_tags($product_ref, $product_ref->{lc}, "categories"); | |||
extract_ingredients_from_text($product_ref); | |||
extract_ingredients_classes_from_text($product_ref); |
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.
maybe we should add a comment at the place in the code that this test mimic to remember that if it is changed, it must be echoed here.
Kudos, SonarCloud Quality Gate passed! |
Added some newly listed additives from EU regulation: https://eur-lex.europa.eu/legal-content/EN/TXT/?uri=CELEX%3A02008R1333-20230720
Added detection of non nutritive sweeteners (needed for new Nutri-Score formula)