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

feature_2/#210 - add nutrient order and list #272

Merged
merged 3 commits into from Nov 10, 2021

Conversation

monsieurtanuki
Copy link
Contributor

Impacted files:

  • openfoodfacts.dart: created method getOrderedNutrients
  • ordered_nutrient_test.dart: used new method OpenFoodAPIClient.getOrderedNutrients; refactored

Impacted files:
* `openfoodfacts.dart`: created method `getOrderedNutrients`
* `ordered_nutrient_test.dart`: used new method `OpenFoodAPIClient.getOrderedNutrients`; refactored
@monsieurtanuki monsieurtanuki marked this pull request as draft October 19, 2021 08:33
@monsieurtanuki
Copy link
Contributor Author

Converted to draft because as long as openfoodfacts/openfoodfacts-server#5997 is not fixed, the nutrient order provided has definitely no added value.
In addition to that, #233 may provide us with a real "country" parameter instead of String cc - if we can avoid the tap-dancing sequence of deprecating String cc while introducing the new country parameter, all the better!

@monsieurtanuki
Copy link
Contributor Author

Now that openfoodfacts/openfoodfacts-server#5997 seems to be fixed and in prod, feel free to review.

Copy link
Member

@M123-dev M123-dev left a 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,
Copy link
Member

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

Copy link
Contributor Author

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.

Copy link
Member

@M123-dev M123-dev Nov 10, 2021

Choose a reason for hiding this comment

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

Okay makes sense

Comment on lines 879 to 885
if (response.statusCode != 200) {
return null;
}
final json = jsonDecode(response.body);
return OrderedNutrients.fromJson(json);
} catch (exception) {
return null;
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Member

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

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

Successfully merging this pull request may close these issues.

None yet

2 participants