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: add localized additives and allergens #929

Conversation

goerlitz
Copy link
Contributor

What

Add support for getting localized additives and allergens

Fixes

#928

@goerlitz goerlitz requested a review from a team as a code owner May 15, 2024 11:34
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 @goerlitz!
I suspect you haven't formatted your code AND you haven't either run the code generation process: dart run build_runner build.
Please run it and push your code.

@goerlitz
Copy link
Contributor Author

Hi @goerlitz! I suspect you haven't formatted your code AND you haven't either run the code generation process: dart run build_runner build. Please run it and push your code.

Ooops, was a formatting issue. Should be fixed now.

@monsieurtanuki
Copy link
Contributor

@goerlitz We may look rather relaxed about failing tests, but it's just an impression.
Therefore, a passing test would be appreciated:
https://github.com/openfoodfacts/openfoodfacts-dart/actions/runs/9097111898/job/25005787033?pr=929
image

@goerlitz
Copy link
Contributor Author

@goerlitz We may look rather relaxed about failing tests, but it's just an impression. Therefore, a passing test would be appreciated: https://github.com/openfoodfacts/openfoodfacts-dart/actions/runs/9097111898/job/25005787033?pr=929 image

I'll take a look. When I ran dart test locally, there were so many failing tests that I did not see this in the output.

@monsieurtanuki
Copy link
Contributor

I'll take a look. When I ran dart test locally, there were so many failing tests that I did not see this in the output.

I can understand that.
Just focus on "your" tests for the moment.

@goerlitz
Copy link
Contributor Author

Alright, the test did not fetch all data needed for the comparison. Fixed now.

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.

LGTM @goerlitz, thank you and congratulations for your first contribution here!

For the record you should have run the JSON related code generator instead of writing into a // GENERATED CODE - DO NOT MODIFY BY HAND file.
Your code will probably be replaced by the generated code anyway, though from what I read it's highly likely that there will be no real difference.

@monsieurtanuki monsieurtanuki merged commit 9d17073 into openfoodfacts:master May 15, 2024
2 of 3 checks passed
@goerlitz
Copy link
Contributor Author

LGTM @goerlitz, thank you and congratulations for your first contribution here!

thanks

For the record you should have run the JSON related code generator

I actually ran dart run build_runner build and did not add it manually. ;)

@monsieurtanuki
Copy link
Contributor

I actually ran dart run build_runner build and did not add it manually. ;)

I assume that the code generator generates a formatted result. And as your push is named "fix code formatting", it gives the impression that you manually formatted the code, not that you used the code generator.
Anyway, if you used the generator in the end that's good! ;)

@goerlitz
Copy link
Contributor Author

I actually ran dart run build_runner build and did not add it manually. ;)

I assume that the code generator generates a formatted result. And as your push is named "fix code formatting", it gives the impression that you manually formatted the code, not that you used the code generator. Anyway, if you used the generator in the end that's good! ;)

Ah, that's what you mean.
Actually, I did not use dart format (or auto-formatting) initially. That's why there was an indentation issue in the non-generated file.

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.

3 participants