Skip to content

feat: add romansh (DEV-867) - #193

Merged
jnussbaum merged 7 commits into
mainfrom
wip/DEV-867-add-romansh
Jun 1, 2022
Merged

jnussbaum merged 7 commits into
mainfrom
wip/DEV-867-add-romansh

Conversation

@jnussbaum

@jnussbaum jnussbaum commented May 24, 2022 •

Copy link
Copy Markdown
Collaborator

resolves DEV-867
resolves DEV-694

@jnussbaum

Copy link
Copy Markdown
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.

@jnussbaum jnussbaum self-assigned this May 24, 2022

@irinaschubert irinaschubert left a comment

Copy link
Copy Markdown

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?

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

@BalduinLandolt BalduinLandolt left a comment

Copy link
Copy Markdown
Collaborator

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
Copy Markdown
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
Copy Markdown

@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!

@sonarqubecloud

sonarqubecloud Bot commented Jun 1, 2022

Copy link
Copy Markdown

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.

3 participants