-
-
Notifications
You must be signed in to change notification settings - Fork 67
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
feature_2/#210 - add nutrient order and list #272
feature_2/#210 - add nutrient order and list #272
Conversation
Impacted files: * `openfoodfacts.dart`: created method `getOrderedNutrients` * `ordered_nutrient_test.dart`: used new method `OpenFoodAPIClient.getOrderedNutrients`; refactored
Converted to draft because as long as openfoodfacts/openfoodfacts-server#5997 is not fixed, the nutrient order provided has definitely no added value. |
Now that openfoodfacts/openfoodfacts-server#5997 seems to be fixed and in prod, feel free to review. |
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.
Left two small comments, but the majority looks good
/// [cc] is the country code, as ISO 3166-1 alpha-2 | ||
static Future<OrderedNutrients?> getOrderedNutrients({ | ||
required final String cc, | ||
required final OpenFoodFactsLanguage language, |
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.
Shouldn't we keep all the features in sync and allow to pass a list instead. Is a list of languages even supported on the server
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.
In first approach I don't think it would make sense to support several languages at the same time: here we're talking about nutrients, not localized food or ingredient names where we would need fallbacks.
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.
Okay makes sense
lib/openfoodfacts.dart
Outdated
if (response.statusCode != 200) { | ||
return null; | ||
} | ||
final json = jsonDecode(response.body); | ||
return OrderedNutrients.fromJson(json); | ||
} catch (exception) { | ||
return null; |
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.
We probably don't need to give detailed error messages since the package user doesn't provide any custom info themselves but at least when a error occurs we should throw the exception, shouldn't we?
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.
Fair enough. I've just pushed fixes - please review them.
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, feel free to merge
Impacted files:
openfoodfacts.dart
: created methodgetOrderedNutrients
ordered_nutrient_test.dart
: used new methodOpenFoodAPIClient.getOrderedNutrients
; refactored