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

Catalog search with zero count #686

Merged
merged 2 commits into from
Jan 22, 2022
Merged

Conversation

veloman-yunkan
Copy link
Collaborator

Fixes #684

@codecov
Copy link

codecov bot commented Jan 21, 2022

Codecov Report

Merging #686 (b8328a7) into master (7440652) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #686   +/-   ##
=======================================
  Coverage   60.79%   60.80%           
=======================================
  Files          54       54           
  Lines        3943     3944    +1     
  Branches     1993     1994    +1     
=======================================
+ Hits         2397     2398    +1     
  Misses       1545     1545           
  Partials        1        1           
Impacted Files Coverage Δ
src/server/internalServer.cpp 82.60% <100.00%> (+0.03%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7440652...b8328a7. Read the comment docs.

@kelson42
Copy link
Collaborator

@veloman-yunkan No sure about your opinion, but I would add a code comment explaining that this is temporary, because of #684 and that this should be removed in a near future.

@veloman-yunkan
Copy link
Collaborator Author

@veloman-yunkan No sure about your opinion, but I would add a code comment explaining that this is temporary, because of #684 and that this should be removed in a near future.

I don't see why this fix should be temporary. The change in the handling of count=0 by the /catalog/search endpoint was unintentional (just a by-product of some code refactoring in 07252a1 #492). I don't see any problem in reversing that change in behaviour.

@kelson42
Copy link
Collaborator

@veloman-yunkan OK, not sure this is the best approach, but considering that requesting 0 books ins anyway quite a non-sense, I won't argue about that. So, thx for that fix.

What about the differrence of behaviour between empty string and undefined? Is that on purpose? What are the rationals?

@veloman-yunkan
Copy link
Collaborator Author

What about the differrence of behaviour between empty string and undefined? Is that on purpose? What are the rationals?

@kelson42 The default value for count was set to 10 and that was on purpose. Handling of count= (empty) is unspecified and I don't think that it should be specified (just like we don't specify the handling of, for example, count=ten or count=half_of_all_books).

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

Successfully merging this pull request may close these issues.

URGENT: Kiwix-desktop 2.1.0 can't download anything online (with latest kiwix-serve at library.kiwix.org)
2 participants