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

Preserve unicode when slugifying by default #797

Merged
merged 1 commit into from
Apr 25, 2022
Merged

Preserve unicode when slugifying by default #797

merged 1 commit into from
Apr 25, 2022

Conversation

rtpg
Copy link
Contributor

@rtpg rtpg commented Apr 21, 2022

Thanks to @hwshim810 (for original implementation) and @lucemia
(for ping + pointing out the perf weirdness) from #572 for motivating this.

This change is motivated by the many many people using non-latin
languages who experience the (rather baffling) behavior of outright
stripping characters when generating slugs from stuff that doesn't
fit into ASCII.

We went through loads of pain as a programming community to get to
nicely supporting unicode everywhere, it's time to take advantage of
that fact and just let people have stuff appear in their native
language as much as possible.

@codecov
Copy link

codecov bot commented Apr 21, 2022

Codecov Report

Merging #797 (cb9c75b) into master (83fe619) will increase coverage by 0.02%.
The diff coverage is 100.00%.

❗ Current head cb9c75b differs from pull request most recent head ddb5ce6. Consider uploading reports for the commit ddb5ce6 to get more accurate results

@@            Coverage Diff             @@
##           master     #797      +/-   ##
==========================================
+ Coverage   92.91%   92.94%   +0.02%     
==========================================
  Files           9        9              
  Lines         720      723       +3     
  Branches      140      141       +1     
==========================================
+ Hits          669      672       +3     
  Misses         34       34              
  Partials       17       17              
Impacted Files Coverage Δ
taggit/models.py 98.29% <100.00%> (+0.04%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 83fe619...ddb5ce6. Read the comment docs.

@rtpg rtpg force-pushed the unicode-slugify branch 3 times, most recently from 43420de to 2626e23 Compare April 21, 2022 14:32
@rtpg
Copy link
Contributor Author

rtpg commented Apr 22, 2022

@lucemia would you be able to review thise changeset? given this is a pretty big change (in few lines of code of course) I do want to have at least one review from someone else

@lucemia
Copy link

lucemia commented Apr 22, 2022

@lucemia would you be able to review thise changeset? given this is a pretty big change (in few lines of code of course) I do want to have at least one review from someone else

Would love to, but I am not reviewer of this repo... is there a way to become a reviewer?

@rtpg
Copy link
Contributor Author

rtpg commented Apr 22, 2022

Two options:

  • you can just look at the code and make a comment. I want somebody to look over it but I am totally comfortable hitting the merge button if it looks good myself. You don't need to be a "real" reviewer.
  • you can join the jazzband org (see here for more details) and do the review "for real"

@lucemia lucemia self-requested a review April 22, 2022 01:22
CHANGELOG.rst Outdated Show resolved Hide resolved
@lucemia
Copy link

lucemia commented Apr 22, 2022

Two options:

  • you can just look at the code and make a comment. I want somebody to look over it but I am totally comfortable hitting the merge button if it looks good myself. You don't need to be a "real" reviewer.
  • you can join the jazzband org (see here for more details) and do the review "for real"

I joined Jazzband! thanks your advice

@rtpg
Copy link
Contributor Author

rtpg commented Apr 25, 2022

#798 this PR seems to indicate another important change that might be necessary here: allow_unicode=True for SlugField. Will look at this issue to see if something should be done here

@rtpg rtpg force-pushed the unicode-slugify branch 3 times, most recently from 8b0dd4c to db41c60 Compare April 25, 2022 05:03
 This change is motivated by the many many people using non-latin
languages who experience the (rather baffling) behavior of outright
stripping characters when generating slugs from stuff that doesn't
fit into ASCII.

 We went through loads of pain as a programming community to get to
nicely supporting unicode everywhere, it's time to take advantage of
that fact and just let people have stuff appear in their native
language as much as possible.
@rtpg rtpg force-pushed the unicode-slugify branch from db41c60 to ddb5ce6 Compare April 25, 2022 05:03
@rtpg
Copy link
Contributor Author

rtpg commented Apr 25, 2022

Alright I made sure that SlugField would accept unicode, and improved the documentation. @lucemia if you could look over this one last time and we can merge it and then I'll prepare a release

@lucemia
Copy link

lucemia commented Apr 25, 2022

I checked the PR, it looks good to me!
Thanks your @rtpg nice work!

@rtpg rtpg merged commit 191d727 into master Apr 25, 2022
@gasman gasman mentioned this pull request Apr 28, 2022
gasman added a commit to gasman/wagtail that referenced this pull request Apr 28, 2022
The next release of django-taggit [will change slugs to allow_unicode=True](jazzband/django-taggit#797), which breaks our check for missing migrations.

This change is not released yet, but the fix is needed now so that we can run against django-taggit git master for our tests against Django main. It's also dependent on the version bump happening at the django-taggit end: jazzband/django-taggit#800
gasman added a commit to wagtail/wagtail that referenced this pull request May 2, 2022
* Fix test migrations for django-taggit 3.0.0 (forthcoming)

The next release of django-taggit [will change slugs to allow_unicode=True](jazzband/django-taggit#797), which breaks our check for missing migrations.

This change is not released yet, but the fix is needed now so that we can run against django-taggit git master for our tests against Django main. It's also dependent on the version bump happening at the django-taggit end: jazzband/django-taggit#800

* Allow django-taggit 3.x as a dependency and drop special case when testing against Django main
@rtpg rtpg deleted the unicode-slugify branch November 18, 2022 04:47
@jsonn
Copy link

jsonn commented Apr 8, 2023

Are you aware that the test case here is broken with Python 3.8?

@rtpg
Copy link
Contributor Author

rtpg commented Apr 10, 2023

@jsonn Python 3.8 is within the test suite and is passing, do you have a stack trace showing failure that can be tracked down here?

@jsonn
Copy link

jsonn commented Apr 10, 2023

Traceback (most recent call last):
  File "/tmp/scratch/www/py-django-taggit/work/django-taggit-3.1.0/tests/test_models.py", line 27, in test_old_slugs
    self.assertEqual([tag.slug for tag in sample_obj.tags.all()], [""])
AssertionError: Lists differ: ['ai-ueo'] != ['']

First differing element 0:
'ai-ueo'
''

- ['ai-ueo']
+ ['']

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