-
-
Notifications
You must be signed in to change notification settings - Fork 383
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: dq_kcal_does_not_match_exclude_more #9339
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #9339 +/- ##
==========================================
+ Coverage 48.18% 48.81% +0.63%
==========================================
Files 65 66 +1
Lines 20348 20355 +7
Branches 4932 4883 -49
==========================================
+ Hits 9805 9937 +132
+ Misses 9288 9161 -127
- Partials 1255 1257 +2 ☔ View full report in Codecov by Sentry. |
lib/ProductOpener/DataQualityFood.pm
Outdated
if ( | ||
( | ||
not((defined $ignore_energy_calculated_error) and ($ignore_energy_calculated_error eq 'yes')) | ||
and ( ($product_ref->{nutriments}{"energy-kj_value"} > 55) |
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.
Can you add a comment to explain why we have this condition on energy? (so that we know if we revisit it later...)
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.
Instead of disabling the error, I think it would be better to change a bit the condition for the error, in case the energy is low.
(in the part # Compare to specified energy value with a tolerance of 30% + an additiontal tolerance of 5)
That way if the energy is 5, but it should be 1500, we want to have the error raised. We should add a test for that (low energy but lots of fat and sugar for instance)
Very good suggestion. Now, we consider either $specified_energy > 55 or $computed_energy > 55 Now, we can also ignore my previous comment in first message: Existing test, already covers that case:
|
Kudos, SonarCloud Quality Gate passed! |
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.
Looks good, thank you!
What
Need to be reviewed carefully.
Related issue(s) and discussion