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.

Sign up for GitHub

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

Jump to bottom

feat: Integrate all routes from nutripatrol's API #991

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

feat: Integrate all routes from nutripatrol's API #991

wants to merge 13 commits into from

Conversation

Valimp
Copy link
Collaborator

@Valimp Valimp commented Nov 13, 2024

What

Add nutripatrol's routes to the dart SDK

Fixes bug(s)

#885

@Valimp Valimp changed the title Integrate all routes from nutripatrol's API feat: Integrate all routes from nutripatrol's API Nov 13, 2024
monsieurtanuki
monsieurtanuki reviewed Nov 13, 2024
View reviewed changes
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 @Valimp!
Please have a look at my comments.

example/lib/main.dart Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Quickly said: test your code in test files, not in main.

lib/openfoodfacts.dart Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

You'll probably need to add the other nutripatrol classes.

lib/src/nutripatrol/create_flag.dart Outdated Show resolved Hide resolved
lib/src/nutripatrol/create_flag.dart Outdated Show resolved Hide resolved
lib/src/nutripatrol/create_flag.dart Outdated Show resolved Hide resolved
lib/src/nutripatrol/create_flag.dart Outdated Show resolved Hide resolved
lib/src/nutripatrol/get_ticket.dart Outdated Show resolved Hide resolved
lib/src/nutripatrol/get_ticket.dart Outdated Show resolved Hide resolved
lib/src/nutripatrol_api_client.dart Outdated Show resolved Hide resolved
@g123k g123k marked this pull request as ready for review January 10, 2025 09:31
@g123k g123k requested a review from a team as a code owner January 10, 2025 09:31
@g123k g123k requested a review from monsieurtanuki January 10, 2025 09:31
monsieurtanuki
monsieurtanuki requested changes Jan 10, 2025
View reviewed changes
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 @Valimp!
The code must pass the analysis tests.

@g123k
Copy link
Contributor

g123k commented Jan 10, 2025

@Valimp will have his midterms next week, that's why I've fixed the main warnings.
However the enum NutripatrolSource is declared twice.

Other than that, I think you can review the PR @monsieurtanuki

@monsieurtanuki
Copy link
Contributor

@Valimp will have his midterms next week
Other than that, I think you can review the PR

@g123k As you may know currently I don't have a computer, so code reviews are sometimes acrobatic and I don't want to review too often.
Besides, I already had a quick look at the code and I saw several things to fix. If @Valimp is busy, I don't see the point of reviewing now instead of when @Valimp is available.
Unless you're in a hurry and want to fix the code yourself.

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

@monsieurtanuki monsieurtanuki monsieurtanuki requested changes

Requested changes must be addressed to merge this pull request.

Assignees

@Valimp Valimp

Labels
🛡️ Nutri-Patrol
Projects
Dart package - Support for new APIs and issues
Status: Review in progress
🧃🛠️ Open Food Facts SDK tracking
Status: Todo
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@Valimp @g123k @monsieurtanuki @teolemon