Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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: Support for unit name normalization #6878
feat: Support for unit name normalization #6878
Changes from 15 commits
31f5d65
af79c44
2649270
d8d91fa
39ec276
98a738e
42beff1
bcef31d
ebe74fc
9d07b03
b4f626e
45bd189
fcf7b9b
73ab7cb
2c34990
ca65acf
e247f36
68638b4
faffdb3
b94ceb2
3d637f5
fdf5820
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
This is dangerous, as it changes the behaviour of the function. We can change the behaviour, but then that means we need to be absolutely certain that we are covering all cases.
If we do want to change it, then I think the function should return "undef" it this case, but not 0.
We also need to test all units that can be passed, in particular in the product edit form on the website.
I tested some and when the unit is "% vol" or "%", the value is converted to 0:
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.
@stephanegigandet The updated PR has tests for these units
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.
Hi @yuktea , we need to be careful: if we don't recognize the unit, we don't want to return a serving size (in fact today we return the value 0).
But the proposed change would mean that something like "43 somethingwedonotunderstand" would result in a serving size of 43 (we would assume the unit is g).
One way to catch this would be to add tests like those:
is( normalize_serving_size("43 unknownthingorunit"), 0 );
is( normalize_serving_size("43 unknownthingorunit (200g)"), 200 );
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 adjusted the logic in the
unit_to_g
so we always check for unit validity at that stage instead of assuming we've already filtered out invalid units. I'll add these tests as you recommend.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.
Do we still need a normalize_unit() function if the translations are in %unit_conversion_map ?