-
-
Notifications
You must be signed in to change notification settings - Fork 56
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
Support for multilang ZIMs #904
Conversation
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## main #904 +/- ##
==========================================
+ Coverage 72.00% 72.11% +0.11%
==========================================
Files 54 54
Lines 3751 3766 +15
Branches 2089 2100 +11
==========================================
+ Hits 2701 2716 +15
Misses 1048 1048
Partials 2 2
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. ☔ View full report at Codecov. |
While this PR indeed fix #903, it seems a bit too simple to fully support multilang ZIMs. (And I would be pleasantly surprised if it is enough) At least, in And I think we should change |
What stemmer should be used for a multilang book? Currently, in such cases the stemmer is simply not used since an attempt to create a stemmer for a non-existing language fails with an exception which is intercepted and ignored.
Will fix.
Should we do that in one step? Or add |
Another option is to preserve |
f87d7b4
to
c7c4655
Compare
Agree with the current code. I think it would be better to depreciate |
@mgautierfr OK, I will deprecate it. Should its current implementation remain unchanged (i.e. should it keep returning a comma separated list of languages for multilang ZIMs)? The other alternatives are:
|
@mgautierfr And what about |
Yes. We keep the function (as deprecated) to not break the api, so we shouldn't change the behavior (even if it is buggy)
Bookmark is about only one article. We can expect that even if zim file is multilanguages, one article is in one language only, so no change needed. |
1f3472b
to
657a294
Compare
`Book::getLanguages()` is used instead of `Book::getLanguage()` when determining the set of languages for a collection of books.
Introduced `Book::getCommaSeparatedLanguages()` instead.
657a294
to
eb002ae
Compare
Fixes #903
Note that a multilanguage ZIM/book is counted as 1 full book in the results to each of the matching
/catalog/v2/entries?lang=<LANG>
queries.