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

OpenFoodAPIConfiguration.globalUriProductHelper ? #813

Open
mopi1402 opened this issue Oct 11, 2023 · 3 comments
Open

OpenFoodAPIConfiguration.globalUriProductHelper ? #813

mopi1402 opened this issue Oct 11, 2023 · 3 comments

Comments

@mopi1402
Copy link

Good evening,

I am not too convinced by the uriProductHelper that takes the default value "prod" for each API.

2 use cases:

  • In general, it is during development that we are most likely to make mistakes (loops, sending mocks, etc.). So, if I had to choose, I would rather send to the sandbox than to production :D

  • It is quite disruptive to no longer be able to configure a global value (used by default). During development, we can find ourselves saving data on the development environment, then trying to read it on the production environment, for example, if we did not pay attention to passing the correct uriProductHelper.
    I don't know how you manage environments on your side, but in my case, I created 2 flavors in my app, one for development and one for production. So, I retrieve the flavor to know which environment to hit.

Being able to change the URLs to hit a local server or other is great, but I would have still kept something like this:

OpenFoodAPIConfiguration.globalUriProductHelper

Then, for each API, I would have put in priority order:

  • The one passed as a parameter in the function (ex: SaveProduct)
  • The one passed in OpenFoodAPIConfiguration.globalUriProductHelper
  • The default value "uriHelperFoodProd" or even "uriHelperFoodTest" to avoid flooding the server during development
@monsieurtanuki
Copy link
Contributor

Hi @mopi1402!

I understand your use-cases.

We had to change our way of dealing with "root urls" for a new feature: being able to check on another server if the product is there ("it's a food app, but perhaps the user mistakenly scanned a product that is already in open beauty facts").

Currently, you can set a static uriProductHelper of yours at app init stage, and pass it as parameter for each method.

If I had to go in your direction, I would totally remove the default value, and force developers to always explicitly set the root url.

Actually I had in mind something different: stop playing with static fields (which are rarely acclaimed as "best practices" in OOP) and merge OpenFoodAPIConfiguration into OpenFoodAPIClient (that would not be made of static methods anymore either). Like, "I have an instance of OpenFoodAPIClient and I call methods for this instance".
Or something like that. But currently I have other priorities, and it would only be a clean OOP refactoring without much added value, feature-wise. And I would like to see what we do with open beauty/pet food/product facts beforehand.

Does that make sense?

@M123-dev
Copy link
Member

@monsieurtanuki from an OOP standpoint it makes total sense, but it would mean a VERY big breaking change, until then I understand the wish to not have manually pass it to every method, most sdk users won't care about which URL to be used

@monsieurtanuki
Copy link
Contributor

@M123-dev I have no intention to switch from static to instance methods in short term.
Again, that would rather make sense when (if?) we have a clear view about OBF OPF OPFF. Which is currently not the case.

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

No branches or pull requests

3 participants