fix: correct nesting of cgi/nutrient.pl API response #6031
Conversation
There was a problem hiding this comment.
Looks very familiar ;)
Beyond my minor comments for nutrients.pl, there's also something to fix in Food.pm that I already mentioned:
'fiber', # should probably be '-fiber'
'--soluble-fiber-', # or should be '-soluble-fiber-'
'--insoluble-fiber-', # and '-insoluble-fiber-'Without that, soluble and insoluble fibers are linked to the latest level 1 parent, which is polyols. Instead of fibers.
cgi/nutrients.pl
Outdated
| else { | ||
|
|
||
| if ($prefix_length == 0) { | ||
| # I'm on level 2, my parent is the latest level 1 parent |
There was a problem hiding this comment.
Confusion with the comments.
There was a problem hiding this comment.
Fixed, thanks
| elsif ($prefix_length == 1) { | ||
| # I'm on level 0, I have no parent, and I'm the latest level 0 parent | ||
| @{$parent_level0->{nutrients}} = () unless defined $parent_level0->{nutrients}; | ||
| push @{$parent_level0->{nutrients}}, $current_ref unless not defined $current_ref; |
There was a problem hiding this comment.
Just curious from my no-perl background: how could $current_ref not be defined?
There was a problem hiding this comment.
Good point. I just changed that code a bit.
Indeed. :)
Thank you @monsieurtanuki , now fixed as well. |
There was a problem hiding this comment.
Hi @stephanegigandet!
Regarding the "fiber fix", in some cases it looks strange to have the fiber and the sub fibers at the same level.
Please have a look at my detailed comments, and ignore them if they're not relevant.
lib/ProductOpener/Food.pm
Outdated
| '-soluble-fiber-', | ||
| '-insoluble-fiber-', |
There was a problem hiding this comment.
Possible, but this time fiber was already at level 1, so my assumption is that the "sub-fibers" should stay on level 2. Or fiber should be at level 0. Probably.
And is it right to put fibers under carbohydrates?
There was a problem hiding this comment.
You are right, I made a search and replace too quickly.
Regarding fibers, in the US they are counted as carbohydrates: https://www.accessdata.fda.gov/scripts/interactivenutritionfactslabel/assets/InteractiveNFL_TotalCarbohydrate_March2020.pdf
One issue we have is that on OFF, we have only one "carbohydrates" field, but in Europe the law says it does not include fibers, while in the US it does. We don't have a good way to handle this difference (or we would need 2 different fields I guess).
There was a problem hiding this comment.
I would imagine 2 "virtual" nutrients, "US-style-carbohydrates" and "non-US-style-carbohydrates", that would be computed from carbohydrates, fibers and nutrition country.
A bit like what could be done between salt and sodium if sometimes you know only one of them.
But that's another story...
lib/ProductOpener/Food.pm
Outdated
| '-soluble-fiber-', | ||
| '-insoluble-fiber-', |
lib/ProductOpener/Food.pm
Outdated
| '-fiber', | ||
| '--soluble-fiber-', | ||
| '--insoluble-fiber-', | ||
| '-soluble-fiber-', | ||
| '-insoluble-fiber-', |
|
Kudos, SonarCloud Quality Gate passed!
|
|
Is it compliant with the OFF etiquette for you to merge your own code? May I merge now? |
We don't have hard rules, usually whoever makes the PR merges it after it's been approved. This one is special though as it's mostly your code. So please do merge it :) |
|
@stephanegigandet Ok then! |
Fixes #5997 . Code from @monsieurtanuki , slightly changed.
This is to output correctly nested nutrients.
Demo: https://fr.openfoodfacts.dev/cgi/nutrients.pl (login and password: off )