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

Refactor product preferences and matching into openfoodfacts-dart #140

Closed
monsieurtanuki opened this issue Feb 3, 2021 · 9 comments · Fixed by #274 or #278
Closed

Refactor product preferences and matching into openfoodfacts-dart #140

monsieurtanuki opened this issue Feb 3, 2021 · 9 comments · Fixed by #274 or #278
Assignees

Comments

@monsieurtanuki
Copy link
Contributor

Concerns:

  • match.dart (should probably be renamed, as Match is already in use by flutter) (ProductMatch? AttributeMatch?)
  • probably ranked_product.darttoo
  • user_preferences_model.dart, which includes classes UserPreferencesModel (to be split - e.g. we cannot embed 'assets/metadata/init_preferences_$languageCode.json'anymore) (could be renamed AttributeImportanceManager?) and PreferencesValue (probably to be renamed to something less vague) (AttributeImportance?)
@stephanegigandet
Copy link
Contributor

  • e.g. we cannot embed 'assets/metadata/init_preferences_$languageCode.json

Maybe we can embed them in the lib folder of the openfoodfacts-dart package: https://medium.com/flutter-community/including-assets-in-a-flutter-package-dd4a82a38ca9

@monsieurtanuki
Copy link
Contributor Author

@stephanegigandet Fair enough, that's a possibility. Assuming that at least the English version is loaded.
But more generally, all the tap dancing about should I start with the lib assets, or the app assets, then https, or maybe a file layer in the middle, that init part should not be in the lib but in each app, IMHO. Provided that we make public all the methods to load lib assets, app assets, https and files.

@stephanegigandet
Copy link
Contributor

Hi @monsieurtanuki , I'd like to work on refactoring the product ranking into openfoodfacts-dart, is that ok with you?

@monsieurtanuki
Copy link
Contributor Author

Hi @stephanegigandet! That's fine by me, but if needed I can devote time to this.

What is very important now I think, is to agree that the current class naming stinks (same name as another preexisting classes for Match, very vague name and probably misleading for UserPreferencesModel and PreferencesValue). Which is very problematic for a library.
Refactoring into openfoodfacts-dart seems to be a good opportunity to "fix" that.
My suggestions:

  • Match => ProductMatch? AttributeMatch? ProductAttributeMatch?
  • RankedProduct is good
  • UserPreferencesModel => AttributePreferences? AttributeImportanceManager?
  • PreferencesValue => AttributeImportance?

If we agree on new names, I can refactor smoothie. And then let you insert the new classes into openfoodfacts-dart. Then someone will remove the classes from smoothie and use off-dart's classes instead.
Is that OK with you?

@stephanegigandet
Copy link
Contributor

It would be great to find good names indeed. If we can find names that also match the JS implementation (or we can also change the JS implementation), it will make documentation etc. much easier as well. That's also the reason why I try to have openfoodfacts-dart use the same names as the API, otherwise it makes things difficult to track across implementations in all the different languages.

Match => ProductMatch sounds good to me.

RankedProduct : I'd prefer MatchedProduct (as it's not ranked yet, just scored). But in fact I wonder if we need an extra RankedProduct structure that just contains a Product and a Match. Could we just use the ProductMatch class? We keep the same arguments for the ProductMatch constructor, and it returns a ProductMatch with a product getter. In fact a ProductMatch could be a child class of Product maybe.

UserPreferencesModels : in the JS library I named I simply named it ProductPreferences. Would that be ok?
One thing to note is that in Smoothie the preferences are user specific, but we could also have a vegan app for instance, where the preferences would be app-specific.

PreferencesValue : PreferenceImportance (without a s for Preference)

What do you think?

If we agree on new names, I can refactor smoothie. And then let you insert the new classes into openfoodfacts-dart. Then someone will remove the classes from smoothie and use off-dart's classes instead.
Is that OK with you?

That would be perfect!

@stephanegigandet
Copy link
Contributor

@monsieurtanuki : if you are done with your changes, I can try to move things to openfoodfacts-dart this week-end

@monsieurtanuki
Copy link
Contributor Author

@stephanegigandet That's OK with me. If I find something to be changed, I'll say so in the OFF-dart code review ;)

@stephanegigandet stephanegigandet changed the title Refactor into openfoodfacts-dart Refactor product preferences and matching into openfoodfacts-dart Mar 27, 2021
stephanegigandet added a commit that referenced this issue Apr 6, 2021
…-to-openfoodfacts-dart

Move personal search preferences and match to openfoodfacts-dart #140 (uses new openfoodfacts-dart module, ready for review)
@monsieurtanuki
Copy link
Contributor Author

@stephanegigandet I think we're done with this one, aren't we?

@monsieurtanuki
Copy link
Contributor Author

I think we're done here.

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