-
-
Notifications
You must be signed in to change notification settings - Fork 66
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
Support Nutriment Nutrient Units #871
Comments
Hi @davidpryor!
I'm not sure it does. I think it is read-only.
That's correct.
It's always grams for weights. /// Returns the value in grams of that [nutrient] for that [perSize].
///
/// It won't be grams for very specific nutrients, like [Nutrient.alcohol].
double? getValue(final Nutrient nutrient, final PerSize perSize) // ...
/// Sets the value in grams of that [nutrient] for that [perSize].
///
/// It won't be grams for very specific nutrients, like [Nutrient.alcohol].
Nutriments setValue(
final Nutrient nutrient,
final PerSize perSize,
final double? value,
) // ... Please share with us a new unit test that fails, according to you. |
Hey @monsieurtanuki thank you for your response!
Through my testing with my implementation, it seems this is a writable field: On my new food, I tested setting carbs to 17000 mg
The parameter map generates
and the GET for the product returns
But the doc string also says I originally looked to the Nutrient enum itself since they carry the "typicalUnit" attribute, which I learned was incorrect 😁 Going back to my proposal, the fallback unit could be grams for all but the special cases. This way to ensure backwards compatibility.
I can provide a working test, but not failing since the feature doesn't exist yet 😁 |
@davidpryor If I remember well there was limited added value in also saving the unit, and it was an additional source of confusion.
My guess: nutrients which are not in g, mg and µg.
I didn't know that. Please create a PR to correct the wrong "typicalUnit"s. |
Thanks for the response @monsieurtanuki ! |
That's what I've understood. The problem is that as I've just written before, we decided NOT to let the user set a unit because:
Therefore unless you have a good reason - e.g. you're blocked in your app - I don't really see the point of adding confusion to the package. Or maybe there's something that I don't understand. Again. |
@CharlesNepote The first time I worked in off-dart - a couple of years ago - the "weight'ed" nutrients had to be converted to grams. Points to address:
Beyond that, not setting the unit for weight'ed nutrients (setting in grams, letting the unit as reasonable default) is only possible for nutrients that have only weight units. |
Why - Problem description
The openfoodfacts server supports passing of nutriment units through the following parameter "nutriement__unit". The current version of the SDK does not support passing this parameter to the server. Currently, with this restriction, a user must read through the SDK code and/or server code to find what units the backend is expected nutrients to be passed as.
What - Proposed solution
Expand the current Nutriments.setValue method signature to accept an optional (for backwards compatibility), parameter called "unit" of type Unit?. If a unit is passed, save these in the object state similar to how nutriment values are saved. Otherwise, fall back to the currently existing "typicalUnit" attribute on the Nutrient Enum. This fallback isn't technically needed, but may make it easier to for users reading through the code. An alternate/addition would be to update the Nutrient Enum docstrings with their default unit.
The easiest way to add it in, would be to make another internal map state in the Nutriment class called "_unitMap".
Also, implementing similar enum patterns for the Unit enum, one could move the OFF string representation for each unit onto the enum itself and simplify the serialization.
https://github.com/davidpryor/openfoodfacts-dart/tree/expose-nutrient-unit-parameter
Alternatives you've considered
The text was updated successfully, but these errors were encountered: