Skip to content

remove dependency on unicode-slugify, use Django builtin function #189

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

Merged
merged 1 commit into from
Apr 19, 2024

Conversation

PetrDlouhy
Copy link
Contributor

@PetrDlouhy PetrDlouhy commented Apr 18, 2024

Use django.utils.text.slugify with allow_unicode=True and remove unicode-slugify dependency (which causes conflicts with python-slugify and seems to be unnecessary.

@PetrDlouhy PetrDlouhy marked this pull request as draft April 18, 2024 11:58
@PetrDlouhy PetrDlouhy force-pushed the remove_slugify_dependency branch 2 times, most recently from ee6f28d to 99b30a8 Compare April 18, 2024 12:50
@PetrDlouhy PetrDlouhy marked this pull request as ready for review April 18, 2024 13:00
@PetrDlouhy
Copy link
Contributor Author

@coordt @sinasamavati Could you remember more closely why the unicode-slugify dependency was introduced in #77? It seems, that the allow_unicode=True is sufficient for Django builtin slugify (but might not have been available back in 2013).
There are no tests added in that PR, I could find only this issue that seems to be related: #73

I have added a test with some testing strings in this PR. All of them seems to have the same result as before, but I would be glad if you could suggest more of them.

@coordt
Copy link
Contributor

coordt commented Apr 18, 2024

It was long ago, but I think it was to support Cyrillic alphabets or fix issues with Cyrillic slugs' romanization.

@PetrDlouhy PetrDlouhy force-pushed the remove_slugify_dependency branch from 347b842 to 61ad474 Compare April 18, 2024 13:54
@PetrDlouhy
Copy link
Contributor Author

OK, thanks. I have added the Greek cyrilic to the testing strings. The output seems to be consistent.

@PetrDlouhy PetrDlouhy merged commit c7e14fe into jazzband:master Apr 19, 2024
7 checks passed
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.

2 participants