-
-
Notifications
You must be signed in to change notification settings - Fork 403
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
feat: translations in packagings READ API v3 #7749
Conversation
@@ -1478,23 +1478,23 @@ | |||
"entry_dates_tags" : "--ignore--", | |||
"food_groups_tags" : [], | |||
"forest_footprint_data" : { | |||
"footprint_per_kg" : 0.0132222222222222, | |||
"footprint_per_kg" : "0.0132222222222222", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Integer to string (Perl issue apparently), can live with it :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @stephanegigandet!
I'm not an expert in .pm
(I don't even know what language it is) but I located your adding of the localized label and that looked appropriate.
Other remarks:
- it's really a pity to have some
int
s (not even allint
s) becomingString
s in json - I hope you can roll that back - what happens if we put several comma-delimited languages in
tags_lc
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This gives a strange result : https://world.openfoodfacts.dev/api/v3/product/3335880004112?fields=packagings&tags_lc=fr,en
I think that's an issue in the test framework, I still have numbers when I access the API. |
It's not supported. Adding something like that could cause errors down the line. e.g. if you ask for "fr", you can get "Plastique", and then write it again, by specifying the same tags_lc=fr. If we try to support something like tags_lc=fr,nl,en , then it becomes very messy: we have to specify in the response which it is, and then in an edit interface, we can have a mix of languages, which the user could change. I don't think it's worth it. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
https://world.openfoodfacts.org/data/taxonomies/packaging_materials.json | ||
https://world.openfoodfacts.org/data/taxonomies/packaging_recycling.json | ||
|
||
If the tags_lc field is set, the properties will include a lc_name field with the translation in the requested language. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't you want to add it as a patternProperty in the files ?
Co-authored-by: Alex Garel <alex@garel.org>
Kudos, SonarCloud Quality Gate passed! |
This PR changes the structure of the returned "packagings" structure so that it can include translated name in the language specified by tags_lc (if sent), as described in #7564 and the OpenAPI specification.
Note that this new structure is only returned for v3 requests, in order not to break apps (not sure if there are some that use this field already) that expect the old structure.
Also added tests.