Skip to content
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

Open
davidpryor opened this issue Jan 29, 2024 · 5 comments
Open

Support Nutriment Nutrient Units #871

davidpryor opened this issue Jan 29, 2024 · 5 comments

Comments

@davidpryor
Copy link
Contributor

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".

class Nutriments extends JsonObject {
  ...
  final Map<String, String> _unitMap = <String, String>{};
  ...
  /// Returns the map key for that [nutrient] unit.
  String _getUnitTag(final Nutrient nutrient) => '${nutrient.offTag}_unit';
  ...
    Nutriments setValue(
    final Nutrient nutrient,
    final PerSize perSize,
    final double? value, {
    final Unit? unit = null,
  }) {
    final Unit finalUnit = unit ?? nutrient.typicalUnit;
    _map[_getTag(nutrient, perSize)] = value;
    _unitMap[_getUnitTag(nutrient)] = finalUnit.offValue;
    return this;
  }
  @override
  Map<String, dynamic> toJson() {
    final Map<String, dynamic> result = <String, dynamic>{};
    for (final Nutrient nutrient in Nutrient.values) {
      for (final PerSize perSize in PerSize.values) {
        final String tag = _getTag(nutrient, perSize);
        final double? value = _map[tag];
        if (value != null) {
          result[tag] = value;
        } else if (_map.containsKey(tag)) {
          result[tag] = '';
        }
        final String? unit = _unitMap[_getUnitTag(nutrient)];
        if (unit != null) {
          result[_getUnitTag(nutrient)] = unit;
        }
      }
    }
    return result;
  }

}

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.

- enum Unit { KCAL, KJ, G, MILLI_G, MICRO_G, MILLI_L, L, PERCENT, UNKNOWN }
+ enum Unit {
+   KCAL(offValue: 'kcal'),
+   KJ(offValue: 'kj'),
+   G(offValue: 'g'),
+   MILLI_G(offValue: 'mg'),
+   MICRO_G(offValue: 'mcg'),
+   MILLI_L(offValue: 'ml'),
+   L(offValue: 'liter'),
+   PERCENT(offValue: 'percent'),
+   UNKNOWN(offValue: '');
+ 
+   final String offValue;
+   const Unit({required String this.offValue});
+ }

https://github.com/davidpryor/openfoodfacts-dart/tree/expose-nutrient-unit-parameter

Alternatives you've considered

  1. Require users to determine the default unit required for the off-server and convert their internal representation units to the expected unit. This still needs documentation updates to let the user know what units are needed. Also, it more makes sense to expose the server-api through the client.
  2. Same as the solution above but with a large refactor. One moves responsibilities of serializing an individual nutrient/unit/modifier/etc combo into a new class.
@monsieurtanuki
Copy link
Contributor

Hi @davidpryor!

The openfoodfacts server supports passing of nutriment units through the following parameter "nutriement__unit".

I'm not sure it does. I think it is read-only.

The current version of the SDK does not support passing this parameter to the server.

That's correct.

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.

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.

@davidpryor
Copy link
Contributor Author

Hey @monsieurtanuki thank you for your response!
Let me know your thoughts.

I'm not sure it does. I think it is read-only.
Through my testing with my implementation, it seems this is a writable field:

On my new food, I tested setting carbs to 17000 mg

"carbohydrates_100g" -> 17000.0
"carbohydrates_unit" -> "mg"

The parameter map generates

"nutriment_carbohydrates" -> "17000.0"
"nutriment_carbohydrates_unit" -> "mg"

and the GET for the product returns

"carbohydrates_100g" -> 17.0

It's always grams for weights.

But the doc string also says /// It won't be grams for very specific nutrients, like [Nutrient.alcohol]. which leaves the user wondering "which specific nutrients?"

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.

Please share with us a new unit test that fails, according to you.

I can provide a working test, but not failing since the feature doesn't exist yet 😁

davidpryor@6c18c71

@monsieurtanuki
Copy link
Contributor

@davidpryor If I remember well there was limited added value in also saving the unit, and it was an additional source of confusion.
Currently we're able to save any nutrient value. Your unit ('mg') is not saved on the server, it's just used for a conversion on the server just before saving the data.
Maybe you're ok with "I need to say it's 17000 mg, and then the server says it's 17 (implicitly) grams and does not store that I prefer a display in mg instead of g"
We preferred to say "Just put the value in grams, for that you'll just have to learn how to divide by 1000 or 1000000, and we don't care about your unit".

which leaves the user wondering "which specific nutrients?"

My guess: nutrients which are not in g, mg and µg.

I originally looked to the Nutrient enum itself since they carry the "typicalUnit" attribute, which I learned was incorrect

I didn't know that. Please create a PR to correct the wrong "typicalUnit"s.

@davidpryor
Copy link
Contributor Author

Thanks for the response @monsieurtanuki !
I think we may be talking about two different points now.
What I am requesting is to expose the write API for sending a nutrient's unit. This will allow the server to do the auto conversion to grams.

@monsieurtanuki
Copy link
Contributor

What I am requesting is to expose the write API for sending a nutrient's unit. This will allow the server to do the auto conversion to grams.

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:

  • the normal expectation would be to get the same unit (e.g. mg) when reading the unit after writing it - and it's not the case as you've noticed in your "17000mg => 17g" test.
  • most developers know how to divide by 1000 or 1000000, and they'll have to multiply later anyway "17g => 17000mg"
  • the confusion is not worth it
    • "hey, I said it's 17000mg, why do you say it's 17g instead?"
    • "hey, I have the right to set the unit of proteins in kcal"
    • and so on

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

No branches or pull requests

3 participants