openfoodfacts-dart icon indicating copy to clipboard operation
openfoodfacts-dart copied to clipboard

Further splitting up of openfoodfacts.dart

Open M123-dev opened this issue 2 years ago • 6 comments

As said in #158 openfoodfacts.dart has gotten really big overtime there therefore we decided (for the time beeing) to code the folksonomy engine API into a separate file.

I would suggest that we take this even further. Namely that we move the Robotoff methods to a separate file and separate class like RobotoffApiClient or so and depending on what the opinions are here do it like most dart packages I have seen and use the main file only for exports. This would mean that we move the OpenFoodAPIClient class to its own file. This should not even break anything since the package user only imports the main file with all the exports.

@monsieurtanuki @teolemon @peterwvj what are your opinions on that?

M123-dev avatar Dec 20 '21 16:12 M123-dev

I fully support this. Even if it requires breaking changes.

peterwvj avatar Dec 20 '21 18:12 peterwvj

Currently openfoodfacts.dart weighs 1092 lines of code.

Inside this file, there are 6 robotoff methods (200 lines of code). That means, still 900 lines of code in openfoodfacts.dart if we move away robotoff.

I don't know the exact relationships between OFF, robotoff and folksonomy, and if I decided to create a separate file for folksonomy it was because it seemed to me like a satellite project with different users (maybe I'm wrong). Then that made sense to create a distinct file in the same library. A different library would have been OK too. But if for the library user we're playing - for both OFF and robotoff - with the same data and the same OFF users, I'm not sure it's a good idea to split OFF and robotoff.

I mean, regardless of technical questions on our side, what would make sense for the library user? OFF, robotoff and folksonomy in the same class/file? Or would that bring confusion to mix unrelated methods? OFF, robotoff and folksonomy in different classes/files? Or would that add complexity to library users: after all, should they care - when they code in dart - if for historical reasons the subdomain is different on the server side? That was for the library user side.

Regarding "our" technical side, I'm OK with @M123-dev's suggestions. I'm a bit reluctant with a breaking change, but that would be a good opportunity to test the feat!: keyword and to switch to version 2.0.0.

monsieurtanuki avatar Dec 20 '21 18:12 monsieurtanuki

I would even wait a little longer with the breaking changes. That is, until we have finished most of the restructuring, which hopefully won't take that long. Then we can remove all other pending deprecated fields directly with it.

I understand your point @monsieurtanuki but somehow Robotoff is still a bit different even though it uses the same users. All the methods still have Robotoff in their name. It's probably not helpfull that robotoff is found by sdk users but classes are not designed for that anyway.

M123-dev avatar Dec 20 '21 20:12 M123-dev

This morning I can

  • create robotoff.dart that contains OpenFoodAPIClient's robotoff's 6 methods under new class RobotoffAPIClient
  • keep the 6 methods in OpenFoodAPIClient but link them to RobotoffAPIClient and deprecate them

@M123-dev OK with that?

monsieurtanuki avatar Dec 21 '21 07:12 monsieurtanuki

  • No strong opinion on the matter.
  • Robotoff is the ML middleware of OFF, to get and validate suggestions on a given barcode
  • Folksonomy is key-value model to allow adding more data to products, beyond our traditional model, but it's a separate server from OFF (it will also be extremely useful to model things like electronic devices for Open Products Facts)

teolemon avatar Dec 21 '21 08:12 teolemon

Folksonomy is key-value model to allow adding more data to products, beyond our traditional model, but it's a separate server from OFF (it will also be extremely useful to model things like electronic devices for Open Products Facts)

@teolemon Is that the same Folksonomy database for food, cosmetic and pets, and potentially electronic devices, all based on a non-checked barcode? If so it should be a different library.

monsieurtanuki avatar Dec 21 '21 09:12 monsieurtanuki

From @atharv028's #723 robotoff now has a proper file. No breaking change for the moment, just deprecated methods.

monsieurtanuki avatar Apr 05 '23 08:04 monsieurtanuki