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

feat: #465 - new "limit" parameter for autocomplete suggestions #498

Merged
merged 7 commits into from
Jun 29, 2022

Conversation

monsieurtanuki
Copy link
Contributor

Impacted files:

  • api_get_automcompleted_suggestions.dart: additional test about the new limit parameter.
  • openfoodfacts.dart: new limit parameter for autocomplete suggestions.

What

  • New limit parameter for autocomplete suggestions.
  • Before that, the limit was always 25 suggestions max.
  • Now the limit is a parameter. There's a max limit of 400 set on the server side.

Fixes bug(s)

Part of

monsieurtanuki and others added 6 commits June 7, 2022 22:28
…ment

Impacted files:
* `api_saveProduct_test.dart`: unrelated test skip
* `KnowlegdePanelElement.dart`: `title` is now optional
* `KnowlegdePanelElement.g.dart`: generated
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
# Conflicts:
#	CHANGELOG.md
#	version.txt
…estions

Impacted files:
* `api_get_automcompleted_suggestions.dart`: additional test about the new `limit` parameter.
* `openfoodfacts.dart`: new `limit` parameter for autocomplete suggestions.
@monsieurtanuki monsieurtanuki requested a review from a team as a code owner June 26, 2022 07:50
Impacted file:
* `api_saveProduct_test.dart`
@monsieurtanuki monsieurtanuki requested a review from M123-dev June 29, 2022 05:55
Copy link
Member

@M123-dev M123-dev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @monsieurtanuki looks good, a simple assert for values over 400 could be added but that's not a priority

@monsieurtanuki monsieurtanuki merged commit 4fd1f75 into openfoodfacts:master Jun 29, 2022
@monsieurtanuki
Copy link
Contributor Author

Thank you @M123-dev for your review!
IMHO adding an assert would not be a great idea: we don't want the app to crash because we're not able to deliver on the server side. A warning would make sense - I've put one in the comment, and I don't know how we could add one in the code, given that we don't know which tools the developers use.

@M123-dev
Copy link
Member

M123-dev commented Jun 29, 2022

Flutter enables assertions in debug mode.

AFAIK they are ignored in builds

@monsieurtanuki
Copy link
Contributor Author

@M123-dev To me assert means something is really wrong, like "dividing by zero", and that the algorithm upstream is flawed ("this value should never be 0, or even then, we should have handle the 0 case before").
Here it's just that if ever there are more than 400 items on the server side that match the search, we'll return only (!) 400 items.
Following this logic we should rather assert that the value is positive. Anyway, that's the beauty of open source: if the developers find it strange that a negative value or a value greater than 400 does not fully work, they can have a look at the code.

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

Successfully merging this pull request may close these issues.

Autocomplete suggestions limited to 25: not enough!
2 participants