-
-
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: Synonyms in taxonomized suggestions #9395
Merged
alexgarel
merged 31 commits into
openfoodfacts:main
from
Naruyoko:Naruyoko-synonym-autocompletion
Feb 19, 2024
+217
−35
Merged
Changes from all commits
Commits
Show all changes
31 commits
Select commit
Hold shift + click to select a range
48fd9a9
Prototype synonyms in taxonomized suggestions
Naruyoko a357410
Make searching work with language code
Naruyoko a1fd46b
API is working hard to give 25 suggestions by default. Why not displa…
Naruyoko 2946868
Ignore `get_synonyms` option when caching
Naruyoko 9546996
Send synonyms before normalization
Naruyoko 815974d
Do not show twice if the synonym is canonical
Naruyoko 7ee498d
Prefer earlier synonyms
Naruyoko 6c6bcb3
Reword the description for `matched_synonyms`
Naruyoko 882974e
Fix display when matched = canonical with LC
Naruyoko 75b607d
Apply linter
Naruyoko c721905
Fix JS linting issues
Naruyoko 32d7882
Add documentation for `get_synonyms` option
Naruyoko 822bc31
Update tests
Naruyoko 6138e9e
Fix display of the canonical name with LC
Naruyoko b4da777
Merge remote-tracking branch 'origin/main' into temp
Naruyoko 8047944
Update tests
Naruyoko 5c28d4b
Remove a commented out debug log
Naruyoko 9f29d6b
Update tests
Naruyoko 086a0a0
Merge remote-tracking branch 'origin/main' into Naruyoko-synonym-auto…
Naruyoko eaee004
Revert "Update tests"
Naruyoko 51da77a
Revert "Update tests"
Naruyoko aa89208
Partially revert "Update tests" (suggest.pl)
Naruyoko 07ae844
Preserve the behavior of suggest.pl API (new functionalities in new f…
Naruyoko 9c73b06
Merge remote-tracking branch 'origin/main' into Naruyoko-synonym-auto…
Naruyoko 4b75874
Run Perl linter
Naruyoko 09daf5f
Change the format of matched synonyms to a dictionary
Naruyoko e86988d
Update docs with the new format
Naruyoko aa5444c
Add tests for get_taxonomy_suggestions_with_synonyms (by copying)
Naruyoko 12c20c3
Run Perl linter on the new test
Naruyoko d1742d1
Merge remote-tracking branch 'origin/main' into Naruyoko-synonym-auto…
Naruyoko f779d0a
docs: minor update on suggest api
alexgarel File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
29 changes: 29 additions & 0 deletions
29
...expected_test_results/api_v3_taxonomy_suggestions/allergens-string-fr-o-get-synonyms.json
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,29 @@ | ||
{ | ||
"errors" : [], | ||
"matched_synonyms" : { | ||
"Arachides" : "Cacahouètes", | ||
"Crustacés" : "Homard", | ||
"Fruits à coque" : "Fruits à coque", | ||
"Gluten" : "Orge", | ||
"Lait" : "Lactose", | ||
"Mollusques" : "Mollusques", | ||
"Moutarde" : "Moutarde", | ||
"Poisson" : "Poisson", | ||
"Soja" : "Soja", | ||
"Œufs" : "Œufs" | ||
}, | ||
"status" : "success", | ||
"suggestions" : [ | ||
"Gluten", | ||
"Œufs", | ||
"Arachides", | ||
"Crustacés", | ||
"Fruits à coque", | ||
"Lait", | ||
"Mollusques", | ||
"Moutarde", | ||
"Poisson", | ||
"Soja" | ||
], | ||
"warnings" : [] | ||
} |
Oops, something went wrong.
Oops, something went wrong.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
from what I undertand seeing tests, you did not only change api-v3 but also api-v2 (in api.yml). shan't you change it there also ?
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 comment might not be relevant if we go for a suggestion api v2.
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.
I am not sure where I should separate my changes. Do I copy this part with a new name with the new function exposed only there? If so, do I need to copy this function as well? Or do the changes need to be separated at the level of inner functions, like from here?
I am also not sure how I should change
api.yml
, since it appears mostly empty.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.
@Naruyoko in the code you show, condition should be enough to sort out the cases.
But we could add a parameter to
get_taxonomy_suggestions
and have acgi/suggest_v2.pl
alike suggest.plThere 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.
@Naruyoko, do not hesitate to ping me on slack if it's not clear enough.
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.
By using dict, do you mean like this (which was what I intended by "traspose"):
Or like this:
Or like this:
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.
I like your third version, because it keeps things simple as you can just retrieve the simple suggestion list, and it's easy to find back matched synonym.
Just one point: shan't it be
"matched_synonyms":{"Gluten":["Orge"],…}
as there might be more than one matched synonym (or don't we care because we just return one ? Which is still really ok for me, as it covers the needs).Also is it useful to put entries like "Œufs": "Œufs" inside matched_synonyms or can the API user deduce from the fact that Œufs is not in the synonyms dict that it means it was directly matched ?
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.
I agree with using the third version.
The matched synonym is currently the first matched one of the best match (with "start">"inside">"fuzzy"). I don't see how much useful it is to return all possible matches.
I think it is better to keep
"a":"a"
entries for consistency.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.
I pushed a change with the new format. I will have to update the API docs and create new tests.
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.
Yes, I read your changes, seems perfect 💪
So I wait for the other changes. Don't hesitate to ping me on slack when you are ready @Naruyoko :-)