search

openfoodfacts / openfoodfacts-dart

Open Food Facts API Wrapper
https://pub.dev/packages/openfoodfacts
Apache License 2.0
168 stars 67 forks source link

Further splitting up of openfoodfacts.dart #339

Closed M123-dev closed 1 year ago

M123-dev commented 2 years ago

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?

peterwvj commented 2 years ago

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

monsieurtanuki commented 2 years ago

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.

M123-dev commented 2 years ago

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.

monsieurtanuki commented 2 years ago

This morning I can

@M123-dev OK with that?

teolemon commented 2 years ago
monsieurtanuki commented 2 years ago

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 commented 1 year ago

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