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

feat: Support Nutriment modifiers #910

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from
Draft

feat: Support Nutriment modifiers #910

wants to merge 1 commit into from

Conversation

g123k
Copy link
Contributor

@g123k g123k commented Apr 18, 2024

Hi everyone,

The idea of this PR is to support nutriment modifiers from the JSON.
Here are the different cases:

  • - : no value available on the package
  • > : more than a value
  • < : less than a value
  • ~ : approximately this value

For now this is just a draft (tests are missing for example).
It's basically to receive feedback for the implementation.

A JSON example with all values with a -:

        "nutriments": {
            "alcohol_modifier": "-",
            "carbohydrates_modifier": "-",
            "energy-kcal_modifier": "-",
            "energy-kj_modifier": "-",
            "fat_modifier": "-",
            "fiber_modifier": "-",
            "proteins_modifier": "-",
            "salt_modifier": "-",
            "saturated-fat_modifier": "-",
            "sodium_modifier": "-",
            "sugars_modifier": "-"
        },

Copy link
Contributor

@monsieurtanuki monsieurtanuki left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @g123k!

My main remarks:

  • we would probably be better off with a breaking change, and from now on use only a Map<String, NutrientValue>, to make it obvious for the developers that they can use potentially modified data with getNutrientValue only
  • if instead we keep the old methods (e.g. getValue) as merely "deprecated", the developers will use potentially wrong data. And in that case NutrientValue has no real added value.
  • if we could get rid of French spelling it wouldn't hurt ;)

}
}

enum NutrimentModifier {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Should be spelled NutrientModifier
  • Could implement OffTagged
  • Not sure if we need @JsonValue here, which is redundant with the value field
  • Would rename field value as modifier

@@ -122,3 +194,71 @@ class Nutriments extends JsonObject {

static Map<String, dynamic> toJsonHelper(Nutriments? n) => n?.toJson() ?? {};
}

class NutrimentValue {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Should be spelled NutrientValue
  • But I'm not convinced of the added value of this class.

@@ -1,7 +1,9 @@
import 'nutrient.dart';
import 'per_size.dart';
import 'package:json_annotation/json_annotation.dart';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure we need it.

Comment on lines +22 to +23
_map[tag] = NutrimentValue._fromMap(map, tag);
continue;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe wrong, not sure, depends on the received data:

  1. are the value and the modifier merged together into a single String, like 'sugars_100g': '<0.5'
  2. or do we get the value in a field and the modifier in another, like 'sugars_100g': 0.5,'sugars_100g_modifier':'<'

If it's (2), the continue is wrong.

@g123k
Copy link
Contributor Author

g123k commented Apr 22, 2024

I was hesitating between the two solutions : a full breaking change path or not.
But you're probably right, and we should fully remove the old methods.
I will do that, thanks as well for your feedbacks.

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

Successfully merging this pull request may close these issues.

None yet

3 participants