-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
Use unicode-slugify to generate Version.slug #5186
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
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -26,6 +26,7 @@ | |
|
||
from django.db import models | ||
from django.utils.encoding import force_text | ||
from slugify import slugify | ||
|
||
|
||
def get_fields_with_model(cls): | ||
|
@@ -53,13 +54,15 @@ def get_fields_with_model(cls): | |
|
||
class VersionSlugField(models.CharField): | ||
|
||
"""Inspired by ``django_extensions.db.fields.AutoSlugField``.""" | ||
""" | ||
Inspired by ``django_extensions.db.fields.AutoSlugField``. | ||
|
||
invalid_chars_re = re.compile('[^-._a-z0-9]') | ||
leading_punctuation_re = re.compile('^[-._]+') | ||
placeholder = '-' | ||
fallback_slug = 'unknown' | ||
Uses ``unicode-slugify`` to generate the slug. | ||
""" | ||
|
||
ok_chars = '-._' # dash, dot, underscore | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. From the rfc https://tools.ietf.org/html/rfc1035
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And it should start with a letter There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, I didn't change this behavior. We could consider. Our current regex was working like that:
Basically, it does not allow If we want to be more strict and follow those directions, I suppose we should not allow the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Wait, this only affects the version's slug, not the project's slug. We are safe here, sorry for the noise. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Shouldn't we use the same logic for project slugs if it's not explicitly defined by the user? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, we use this for project's slug https://github.com/stsewd/readthedocs.org/blob/f1fd0c9827c10834277d63aa35cb4511ee3ae5ed/readthedocs/core/utils/__init__.py#L209-L219 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't mean removing the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Because Project's are used in DNS, but Versions aren't. They are different things, so we don't need the exact same restrictions. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, I understand that. I'm suggesting to use the same logic (unicode-slugify to generate the slug) but apply different restrictions. Anyway, if so, it's for a different PR. |
||
test_pattern = re.compile('^{pattern}$'.format(pattern=VERSION_SLUG_REGEX)) | ||
fallback_slug = 'unknown' | ||
|
||
def __init__(self, *args, **kwargs): | ||
kwargs.setdefault('db_index', True) | ||
|
@@ -78,13 +81,37 @@ def get_queryset(self, model_cls, slug_field): | |
return model._default_manager.all() | ||
return model_cls._default_manager.all() | ||
|
||
def _normalize(self, content): | ||
""" | ||
Normalize some invalid characters to become a dash (``-``). | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could use a better docstring -- this tells me what not _why. Is there a reason we only do this for a subset of characters, but not others? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I added the why: which is basically to keep compatibility. |
||
|
||
For example, ``release/1.0`` will become ``release-1.0``. | ||
""" | ||
return re.sub('[/%!?]', '-', content) | ||
|
||
def slugify(self, content): | ||
""" | ||
Make ``content`` a valid slug. | ||
|
||
It uses ``unicode-slugify`` behind the scenes which works properly with | ||
Unicode characters. | ||
""" | ||
if not content: | ||
return '' | ||
|
||
slugified = content.lower() | ||
slugified = self.invalid_chars_re.sub(self.placeholder, slugified) | ||
slugified = self.leading_punctuation_re.sub('', slugified) | ||
normalized = self._normalize(content) | ||
slugified = slugify( | ||
normalized, | ||
only_ascii=True, | ||
spaces=False, | ||
lower=True, | ||
ok=self.ok_chars, | ||
space_replacement='-', | ||
) | ||
|
||
# Remove first character wile it's an invalid character for the | ||
# beginning of the slug | ||
slugified = slugified.lstrip(self.ok_chars) | ||
|
||
if not slugified: | ||
return self.fallback_slug | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -68,6 +68,9 @@ django-kombu==0.9.4 | |
mock==2.0.0 | ||
stripe==2.19.0 | ||
|
||
# unicode-slugify==0.1.5 is not released on PyPI yet | ||
git+https://github.com/mozilla/unicode-slugify@b696c37#egg=unicode-slugify | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🙁 |
||
|
||
django-formtools==2.1 | ||
|
||
# docker is pinned to 3.1.3 because we found some strange behavior | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we should have a different import for this: