-
Notifications
You must be signed in to change notification settings - Fork 108
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
Fix misleading pattern name and documentation #109
Conversation
Currently, mypy understands the type as `Iterable[str]`, which doesn't match what should actually be passed in, which is `Iterable[Iterable[str]]` or, ideally, `Iterable[Tuple[str, str]]`
@mrezzamoradi Thank you for raising this PR. However, the Code: # replace all other unwanted characters
if lowercase:
pattern = regex_pattern or ALLOWED_CHARS_PATTERN
else:
pattern = regex_pattern or ALLOWED_CHARS_PATTERN_WITH_UPPERCASE
text = re.sub(pattern, DEFAULT_SEPARATOR, text). # <-- look here Example: >>> allowedPattern = re.compile(r'[^-a-z0-9]+')
>>> text = 'Aboo$%$%$%9-999--asdfasfd'
>>> unwantedRemoved = re.sub(allowedPattern, '-', text)
>>> print(unwantedRemoved)
'-boo-9-999--asdfasfd'
>>> Also: |
Once again thank you for taking the time to raise this PR. With that said, I am going to closing this PR for now. Should you have time to add github actions to this repo, I would appreciate it as well. |
Thanks for your comment @un33k
I hope this explanation brings some light on the confusion here. The same explanation can explain some part of why I've removed the lowercase regex pattern (because the matching characters would always be a subset of the matching characters when using |
btw this PR passes all the tests |
@mrezzamoradi I enabled github actions to ensure we have a trusted CI. As soon as the tests passed, I decided to pull your request in. I do appreciate your contribution. I have sent a request, should you decide to join this project as a contributor. |
The
ALLOWED_CHARS_PATTERN
andregex_pattern
are actually disallowed characters in the final slug.Also since the code lowers the case in line 141, there's no need for a separate regex pattern for lowercase strings