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

Added command line parameter to not create zim for languages with variants #206

Merged
merged 2 commits into from
Mar 5, 2023

Conversation

pavel-karatsiuba
Copy link
Collaborator

@pavel-karatsiuba pavel-karatsiuba commented Mar 4, 2023

Metadata Language is not supporting language with variants, for example, 'en_CA'.
With this PR added the command line parameter --withoutLanguageVariants which filters the language list and removes languages with variants.
This means that if the user uses the parameter --withoutLanguageVariants then zim files for languages with variants will not be created.

Fix: #204

@kelson42
Copy link
Contributor

kelson42 commented Mar 4, 2023

@pavel-karatsiuba thx but:

  • the PR description does not clearly explain de impact
  • You don't have open and link an other ticket like requested on slack

@@ -81,6 +81,9 @@ const fetchLanguages = async (): Promise<void> => {
rows.forEach((item) => {
const url = $(item).find('td.list-highlight-background:first-child a').attr('href')
const slug = /locale=(.*)$/.exec(url)?.pop()
if (argv.withoutLanguageVariants && slug.includes('_')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should have a log entry

@@ -81,6 +81,9 @@ const fetchLanguages = async (): Promise<void> => {
rows.forEach((item) => {
const url = $(item).find('td.list-highlight-background:first-child a').attr('href')
const slug = /locale=(.*)$/.exec(url)?.pop()
if (argv.withoutLanguageVariants && slug.includes('_')) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It should not be based on the slug, but on the lang. This is not robust.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Slug it just a variable name. It is filled from the locale part of the URL.
All other steps and functions use the value from this variable to define the language.

README.md Outdated
@@ -30,6 +30,13 @@ npm i && npm start
The above will eventually output a ZIM file to ```dist/```

## Command line arguments
`--withoutLanguageVariants` uses to exclude languages with County variant. For example `en_ca` will not be present in zim with this argument.
Copy link
Contributor

Choose a reason for hiding this comment

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

typo

README.md Outdated
@@ -30,6 +30,13 @@ npm i && npm start
The above will eventually output a ZIM file to ```dist/```

## Command line arguments
`--withoutLanguageVariants` uses to exclude languages with County variant. For example `en_ca` will not be present in zim with this argument.
Copy link
Contributor

Choose a reason for hiding this comment

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

is that not "en-CA"?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

From the phet site, we are getting "en_CA"

@@ -81,6 +81,9 @@ const fetchLanguages = async (): Promise<void> => {
rows.forEach((item) => {
const url = $(item).find('td.list-highlight-background:first-child a').attr('href')
Copy link
Contributor

Choose a reason for hiding this comment

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

Please fix problems

Copy link
Contributor

@kelson42 kelson42 left a comment

Choose a reason for hiding this comment

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

LGTM

@kelson42 kelson42 merged commit 188cdb2 into main Mar 5, 2023
@kelson42 kelson42 deleted the create-zim-without-language-variants branch March 5, 2023 10:41
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.

Duplicate phet ZIM files
2 participants