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
Fixed #18119 -- Added a DomainNameValidator validator. #18037
base: main
Are you sure you want to change the base?
Fixed #18119 -- Added a DomainNameValidator validator. #18037
Conversation
Thank you for this Nina ⭐
All new features need some docs and a release note, so this is roughly what I am expecting:
|
fc3bd96
to
f77f5f3
Compare
Thanks Sarah - docs now updated. |
Thanks @felixxm - docs now updated following comments. |
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.
Thank you @nmenezes0 ⭐ I have a few more comments.
Please also squash this to a single commit 👍
b2b5e90
to
151845f
Compare
Override __init__ function to add accept IDNA. Updates to DomainNameValidator to add logic around accept_idna flag Remove outdated details from docs. Made changes to domain name validation and tests. Reused regex in URLValidator. Added more changes to the DomainNameValidator so all tests pass. Removed rogue space. Blacken. Update documentation for DomainNameValidator. Remove unused text. Tidy docs. Update docs - fix underline. Update docs/ref/validators.txt Remove unnecessary blank lines. Co-authored-by: Mariusz Felisiak <felisiak.mariusz@gmail.com> Update docs/releases/5.1.txt Co-authored-by: Mariusz Felisiak <felisiak.mariusz@gmail.com> Changes to docs for DomainNameValidator. Change name of the parameter. Co-authored-by: Sarah Boyce <42296566+sarahboyce@users.noreply.github.com> Update docs for new naming. Co-authored-by: Sarah Boyce <42296566+sarahboyce@users.noreply.github.com> Update django/core/validators.py Co-authored-by: Sarah Boyce <42296566+sarahboyce@users.noreply.github.com> Update comments for validators. Reorder docs to have DomainNameValidator next to URLNameValidator. blacken
151845f
to
dd9816e
Compare
Thanks @sarahboyce - I've made those changes and squashed to a single commit. |
I'm not sure why some of the checks are failing. The error is "commit changed" - potentially due to me pushing the squashed commit? |
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.
Thank you for the updates @nmenezes0, we're really close ⭐
I'm not sure why some of the checks are failing. The error is "commit changed" - potentially due to me pushing the squashed commit?
Don't worry about this, sounds unrelated to your changes
The ticket in Trac suggests that the regex could be reused elsewhere in the codebase. I have reused it in the URLValidator, but I couldn't see how to use it in django/core/mail/message.py or django/utils/html.py.
Read through the original comment made by @claudep 12 years ago.
- It's still valid that we could chose to use this validation in other areas of the codebase.
- I don't think refactoring to include this is in scope of this ticket, however we should make sure it's defined in a good place.
- I'm not sure which
__init__
is being referred to in the comment and I don't get circular imports if I was to importDomainNameValidator
to the mentioned files.
@claudep can you clarify where you think DomainNameValidator
should be defined or whether it is correct to be in django/core/validators.py
?
I think it is ok to be defined here but I might be missing something 👍
Co-authored-by: Sarah Boyce <42296566+sarahboyce@users.noreply.github.com>
Co-authored-by: Sarah Boyce <42296566+sarahboyce@users.noreply.github.com>
Co-authored-by: Sarah Boyce <42296566+sarahboyce@users.noreply.github.com>
Thanks @sarahboyce - have commited those docs changes - I assume I should squash into one commit again (will do that later). |
Trac ticket number
ticket-18119
Branch description
Provide a concise overview of the issue or rationale behind the proposed changes.
This ticket adds a DomainNameValidator to validate International domain names. This validator takes a flag
is_idna
(defaults toTrue
) - to determine whether or not we should use non-ASCII international domain names.This builds on the existing PR with some changes: not using IP addresses (don't think they are part of a domain name), removing changes to docs as out of date (not sure what updates are needed here now?), changes to the tests and how they were written, reusing the regex in URLValidator.
The ticket in Trac suggests that the regex could be reused elsewhere in the codebase. I have reused it in the URLValidator, but I couldn't see how to use it in
django/core/mail/message.py
ordjango/utils/html.py
.Checklist
main
branch.I wasn't sure what changes I should make to the documentation.