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: Refactor Robotoff methods #723

Merged

Conversation

atharv028
Copy link
Contributor

What

  • Deprecated and routed the robotoff apis to a new file for decluttering
  • Changed names of some APIs to maintain consistency in new file
  • No breaking changes for now

Screenshot

Screenshot 2023-04-05 at 12 40 39 AM

Fixes bug(s)

Part of

Deprecated and routed the robotoff apis to a new file for decluttering
@atharv028 atharv028 requested a review from a team as a code owner April 4, 2023 19:14
Removed User parameter as mentioned in the requirements along with getIngredientSpellingCorrection method. Could be a breaking change
@atharv028 atharv028 changed the title Separated Robotoff APIs (initial) Fix: Refactor Robotoff methods Apr 5, 2023
@atharv028 atharv028 changed the title Fix: Refactor Robotoff methods fix: Refactor Robotoff methods Apr 5, 2023
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 @atharv028!
Please have a look at my comments.

@@ -107,4 +107,5 @@ export 'src/utils/tag_type.dart';
export 'src/utils/unit_helper.dart';
export 'src/utils/uri_helper.dart';
export 'src/utils/uri_reader.dart';
export 'src/robotoff_api_client.dart';
Copy link
Contributor

Choose a reason for hiding this comment

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

I would prefer robot_off_api_client.dart but that's not a big deal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@@ -788,226 +789,66 @@ class OpenFoodAPIClient {
}
}

//TODO(x): Add comments for robotoff
//TODO : Remove Robotoff APIs From here
Copy link
Contributor

Choose a reason for hiding this comment

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

That TODO is not very useful, please get rid of ot.

Instead, please add a specific TODO comment for each related method, something like that:
// TODO: deprecated from 2022-12-01; remove when old enough

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added TODO on each method in open_food_api_client.dart which routes to RobotOffAPIClient

@@ -2,6 +2,7 @@ import 'dart:async';
import 'dart:convert';

import 'package:http/http.dart';
import 'package:openfoodfacts/openfoodfacts.dart';
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use src/ imports instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

import 'package:http/http.dart';
import 'package:openfoodfacts/openfoodfacts.dart';

class RobotoffApiClient {
Copy link
Contributor

Choose a reason for hiding this comment

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

For consistency with OpenFoodAPIClient, RobotOffAPIClient would make more sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed the class name to RobotOffAPIClient

import 'dart:convert';

import 'package:http/http.dart';
import 'package:openfoodfacts/openfoodfacts.dart';
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use src/ imports instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed

class RobotoffApiClient {
RobotoffApiClient._();

//TODO(x): Add comments for robotoff APIs
Copy link
Contributor

Choose a reason for hiding this comment

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

Please get rid of that comment.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Deleted the comment


static Future<RobotoffQuestionResult> getProductQuestions(
String barcode,
String lang, {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be OpenFoodFactsLanguage language instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed the parameter type to OpenFoodFactsLanguage and name to language

}

static Future<RobotoffQuestionResult> getRandomQuestions(
OpenFoodFactsLanguage? lang,
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be language.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

Fixed import directives, also made the naming conventions for classes consistent. Added TODOs
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 @atharv028!

@monsieurtanuki monsieurtanuki merged commit 7a71cbb into openfoodfacts:master Apr 5, 2023
2 of 3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants