Navigation Menu

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: add romansh (DEV-867) #193

Merged
merged 7 commits into from Jun 1, 2022
Merged

feat: add romansh (DEV-867) #193

merged 7 commits into from Jun 1, 2022

Conversation

jnussbaum
Copy link
Collaborator

@jnussbaum jnussbaum commented May 24, 2022

resolves DEV-867
resolves DEV-694

@jnussbaum
Copy link
Collaborator Author

@irinaschubert @BalduinLandolt With this PR, I resolved two issues. But I'm not sure if my solution is too superficial, because I only prevented 3-letter-language-codes via the JSON Schema. I don't know if it could/should be prevented on a deeper level.

Copy link

@irinaschubert irinaschubert left a comment

Choose a reason for hiding this comment

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

Regarding your question, there is a file called language-codes-3b2_csv.csv. I think in there the 3 letter codes should be removed as well. I am not sure if in there only en, de, fr, it and rm should be listed or if we need all of the 2 letter codes. This file is used for the validation of language codes that are used in lists which are created from Excel files.
It was me who added this file initially in #75. But I don't remember if it was us who just added it (I remember that I discussed it with Ivan) or if it was a real user requirement to support more languages in lists.
It would therefore be good to discuss it with PM or Rita if we need more than the 5 languages for list labels.
We should also check in the triplestore if we actually have projects that use other than the 5 languages (at least I hope so). If so, could you please add a sub-task and assign me?

docs/dsp-tools-create.md Outdated Show resolved Hide resolved
jnussbaum and others added 2 commits May 25, 2022 11:37
Co-authored-by: irinaschubert <irina.schubert@dasch.swiss>
Copy link
Collaborator

@BalduinLandolt BalduinLandolt left a comment

Choose a reason for hiding this comment

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

So far so good.

I agree that the 3 letter language codes should be removed from the language-codes-3b2_csv.csv file; and this is only used in knora.dsplib.utild.excel_to_json_lists.py in check_language_code() which then should also be adjusted. (The current implementation checks every field in the CSV, which so far has been 3-letter-code, 2-letter-code and country name; after removing the 3-letter-codes, it would check the 2-letter coded and the country names. But in my opinion it should not check the country names at all.).
I'm just unsure if that's in the scope of this PR.

@jnussbaum
Copy link
Collaborator Author

@irinaschubert @BalduinLandolt Following your comments, I suggest removing language-codes-3b2_csv.csv altogether. It doesn't make sense to have tons of language codes that are never used, because everywhere else in the DSP software stack, there are only the hardcoded en, fr, de, it, rm, and nothing else. So I suggest hardcoding it also here. Is that okay for you?

@irinaschubert
Copy link

@irinaschubert @BalduinLandolt Following your comments, I suggest removing language-codes-3b2_csv.csv altogether. It doesn't make sense to have tons of language codes that are never used, because everywhere else in the DSP software stack, there are only the hardcoded en, fr, de, it, rm, and nothing else. So I suggest hardcoding it also here. Is that okay for you?

Yes, that makes sense to me!

@sonarcloud
Copy link

sonarcloud bot commented Jun 1, 2022

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@jnussbaum jnussbaum merged commit 86d3e6a into main Jun 1, 2022
@jnussbaum jnussbaum deleted the wip/DEV-867-add-romansh branch June 1, 2022 13:42
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.

None yet

3 participants