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

Add Yiddish language ruleset #336

Merged
merged 2 commits into from
Dec 25, 2023
Merged

Add Yiddish language ruleset #336

merged 2 commits into from
Dec 25, 2023

Conversation

yankl
Copy link
Contributor

@yankl yankl commented Dec 18, 2023

Added a ruleset for Yiddish, and added it to the default languages, as it's the only language so far here that uses this character set, and for any other languages that use these letters (e.g., Hebrew, Aramaic), it will be much better to have the Yiddish ruleset than nothing.
I also added a test to defaultRuleProvider and ran the tests -- everything still passes.

Copy link
Member

@florianeckerstorfer florianeckerstorfer left a comment

Choose a reason for hiding this comment

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

Great, thank you

@florianeckerstorfer
Copy link
Member

I think you need to run php bin/generate-default.php and commit the generated file

@yankl
Copy link
Contributor Author

yankl commented Dec 23, 2023

Sure, done! Since it's generated automatically I wasn't sure if I was supposed to include it in the PR.

@florianeckerstorfer florianeckerstorfer merged commit 8672df7 into cocur:main Dec 25, 2023
8 checks passed
@florianeckerstorfer
Copy link
Member

@yankl Nice, thank you,

@@ -53,6 +53,7 @@ class Slugify implements SlugifyInterface
// Languages are preferred if they appear later, list is ordered by number of
// websites in that language
// https://en.wikipedia.org/wiki/Languages_used_on_the_Internet#Content_languages_for_websites
'yiddish',

Choose a reason for hiding this comment

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

The whole list has an invalid order.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you explain what you mean? What makes the order invalid?

Choose a reason for hiding this comment

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

Please look at the comment above the list in the code.

list is ordered by number of websites in that language

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, and it also says "Languages are preferred if they appear later" meaning the more popular languages should come last. And as you see, Russian, German and Polish appear near the bottom of the list here, while they're near the top of the list (and in the reverse order) on the wiki page. You're right that the order in general doesn't match. For example, whoever put Romanian at the bottom of the list made a mistake, as it's number 23 on the wiki page, way below German, for example. But I believe I correctly placed Yiddish at the top of the list, giving it very low priority, as it doesn't even appear on the list of most popular languages. And you're making this comment on this commit which only adds Yiddish to the list, hence my confusion. Are you claiming that Yiddish was incorrectly placed or just that the list as a whole is not in the stated order?

Choose a reason for hiding this comment

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

Exactly, the whole list is off the order.

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