-
-
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
Add mappings for languages not given by libicu #701
Conversation
Codecov Report
@@ Coverage Diff @@
## master #701 +/- ##
==========================================
+ Coverage 57.93% 58.03% +0.10%
==========================================
Files 54 54
Lines 3573 3584 +11
Branches 2010 2019 +9
==========================================
+ Hits 2070 2080 +10
- Misses 1502 1503 +1
Partials 1 1
Continue to review full report at Codecov.
|
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.
@juuz0 From a a user perspective it works as expected. I also appreciate you made an effort to identify the other languages impacted by a similar problem. I'll let @veloman-yunkan review from a code perspective, but I believe this would be more efficient to merge iso6391To3.js
and iso639_3.js
, this is a bit a waste of resources to have to make two HTTP requests IMO.
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 think you have missed the fact that loadAndDisplayOptions()
is used for populating not only the list of languages but also the list of categories. Your (already merged) PR #697 suffers from the same problem (and introduces a potential bug). Please try to take the mentioned fact into account adjust this PR accordingly (as well as identify and fix the potential bug introduced by #697).
Done.
Correct, done. |
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.
Should the map be rather moved to C++?
Since the static resources of kiwix-serve are compiled into the executable rather than hosted separately keeping the list of additional language mappings in js file doesn't seem to have any advantage, since we can't deploy it without building (and releasing) a new version of libkiwix and kiwix-tools. Fixing getLanguageSelfName()
in src/opds_dumper.cpp
will be a more coherent implementation.
If we are going to merge this PR with the solution implemented in the frontend, then the order of commits should be reversed:
- first fix the bug introduced earlier
- then make this new enhancement correctly.
IMHO, we better quickly do 1. as a separate PR and quietly decide whether this fix will be in C++ or JS.
I agree with this comment. |
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.
The latest changes are fine, but there is still one unresolved comment.
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.
There is no need to use fixup commit's for this small PR. Please submit the next request for review as a single-commit history.
Still not ready for a new review (for those unresolved comments), will be done by today night I hope. |
Adds a std::map<std::string, std::string> with display names for language codes not given by libicu Fault language codes are taken from library.kiwix.org
Adds dagbani (dag) language in iso639_3 language map
Fixes #611
There are 7,893 languages under ISO 639-3. I could not find a mapping between lang code and native names (there's this langCode vs english names but we display native names).
Instead, this just adds the languages missed by libicu in library.kiwix.org. If this way is approved, I would like to ask: