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

fix: failing nutriment persize serving test #873

Conversation

davidpryor
Copy link
Contributor

What

Fixed inability to send nutrients to server that were tagged with PerSize.serving

Fixes bug(s)

#872

@davidpryor davidpryor requested a review from a team as a code owner January 30, 2024 17:58
@davidpryor davidpryor changed the title Failing nutriment persize serving test fix: failing nutriment persize serving test Jan 30, 2024
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.

Thank you @davidpryor for finding and fixing that bug!
I have comments regarding the code itself though, regarding maintainability.
Please have a look at them and feel free to tell me what you think.

test/api_save_product_test.dart Outdated Show resolved Hide resolved
test/api_save_product_test.dart Outdated Show resolved Hide resolved
test/api_save_product_test.dart Outdated Show resolved Hide resolved
test/api_save_product_test.dart Outdated Show resolved Hide resolved
test/api_save_product_test.dart Outdated Show resolved Hide resolved
test/api_save_product_test.dart Outdated Show resolved Hide resolved
test/api_save_product_test.dart Outdated Show resolved Hide resolved
lib/src/open_food_api_client.dart Outdated Show resolved Hide resolved
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.

Thank you @davidpryor for your changes!

I cannot see the added value of your additional "Test saving food with PerSize.serving Nutrients" test. Now that you loop in the already existing "confirm that nutrient fields are saved" test for all PerSize values, we don't need to copy/paste the test, right?
Explain that to me or fix that, and I'll approve your PR.

Less important: if you used a const map for initial values, you would never have potential discrepancies between the nutrient values you set and the list of nutrients you check:

const Map<Nutrient, double> rootNutrientValues = <Nutrient, double>{
  Nutrient.energyKJ: 365,
  Nutrient.carbohydrates: 12,
  Nutrient.proteins: 6,
  Nutrient.fat: 0.1,
};

E.g. using rootNutrientValues.keys.

lib/src/open_food_api_client.dart Outdated Show resolved Hide resolved
test/api_save_product_test.dart Outdated Show resolved Hide resolved
test/api_save_product_test.dart Outdated Show resolved Hide resolved
test/api_save_product_test.dart Outdated Show resolved Hide resolved
davidpryor and others added 4 commits January 31, 2024 09:45
- import 'package:openfoodfacts/openfoodfacts.dart';
+ import 'model/per_size.dart';

Co-authored-by: monsieurtanuki <fabrice_fontaine@hotmail.com>
Co-authored-by: monsieurtanuki <fabrice_fontaine@hotmail.com>
Co-authored-by: monsieurtanuki <fabrice_fontaine@hotmail.com>
@davidpryor
Copy link
Contributor Author

I cannot see the added value of your additional "Test saving food with PerSize.serving Nutrients" test. Now that you loop in the already existing "confirm that nutrient fields are saved" test for all PerSize values, we don't need to copy/paste the test, right?
Explain that to me or fix that, and I'll approve your PR.

I agree. Just missed deleting it.

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.

Thank you @davidpryor for your PR, looks good to me!

@monsieurtanuki monsieurtanuki merged commit 31f9fab into openfoodfacts:master Jan 31, 2024
2 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants