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

use email_validator library for Email validator instead of regex #429

Merged

Conversation

alanhamlett
Copy link
Member

@alanhamlett alanhamlett commented Jul 5, 2018

Implements outcome of conversation in #427 using email_validator to validate email address.

@alanhamlett alanhamlett force-pushed the ahamlett-email_validation_using_library branch from 4d9e165 to ff2a179 Compare Jul 5, 2018

def __init__(self, message=None):
def __init__(self, message=None, **options):
Copy link
Member

@davidism davidism Jul 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

options needs a mention in the docstring.

@@ -2,6 +2,7 @@

import re
import uuid
from email_validator import validate_email, EmailNotValidError
Copy link
Member

@davidism davidism Jul 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be optional like ipaddress?

Copy link
Member Author

@alanhamlett alanhamlett Jul 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer just adding a hard dependency since email validation is very common, but maybe I don't know the history of why ipaddress is optional. What are the benefits of making it optional? Consistency with ipaddress?

Copy link
Member

@davidism davidism Jul 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you don't use email validation (I don't for many projects), you now have an extra dependency that you don't use. I'd prefer an optional dependency since email validation isn't required to making WTForms work.

I don't recall exactly why, but I think using CC0 for a license is potentially problematic, so forcing users to install email_validator could cause licensing issues as well.

Copy link
Member Author

@alanhamlett alanhamlett Jul 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made optional with a2a7afe. Also, added extras for both email_validator and ipaddress dependencies so they can be installed at the same time as wtforms. Removed locale extra since Babel is already a required dependency, so there's no reason for having it in extras.

@alanhamlett alanhamlett force-pushed the ahamlett-email_validation_using_library branch from ff2a179 to c5ab624 Compare Jul 5, 2018
setup.py Outdated
extras_require={"locale": ["Babel"]},
install_requires=["MarkupSafe"],
extras_require={
"ipaddress": ["ipaddress"],
Copy link
Member

@davidism davidism Jul 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what the syntax is, but this should only apply to Python 2.

Copy link
Member Author

@alanhamlett alanhamlett Jul 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extras are just ways to define optional dependencies, so it's up to the user to install wtforms with various extras or not depending on what they need and their environment. Not sure if we can specify extras only available for Python 2.

Copy link
Member Author

@alanhamlett alanhamlett Jul 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Member

@davidism davidism Jul 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this would work:

'ipaddress:python_version < "3"': ["ipaddress"],

Copy link
Member Author

@alanhamlett alanhamlett Jul 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It works! Tested 50a0c3c on Python 2.7 and Python 3.6 both with pip install -e .[ipaddress] and ipaddress was only installed on Python 2.7.

Copy link
Member Author

@alanhamlett alanhamlett Jul 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, didn't see your last comment. I'll update to < 3.

setup.py Outdated
@@ -100,7 +100,7 @@ class BDistWheel(CompileCatalogMixin, BaseBDistWheel):
setup_requires=["Babel>=2.6.0"],
install_requires=["MarkupSafe"],
extras_require={
"ipaddress": ['ipaddress;python_version<="2.7"'],
"ipaddress": ['ipaddress;python_version<"3"'],
Copy link
Member

@davidism davidism Jul 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Uhh, not to drag this out too long, but technically this should be "<3.3". 😓 Then again, we don't support <3.4, so... 🤷‍♂️

Copy link
Member Author

@alanhamlett alanhamlett Jul 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated with 325367f. 👍

Copy link
Member Author

@alanhamlett alanhamlett Jul 10, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure if you've seen the latest push, and don't want to merge without full review.

@alanhamlett
Copy link
Member Author

@alanhamlett alanhamlett commented Jul 17, 2018

Should be ready to merge now.

One thing to consider is improving install docs to mention extras, but that could be another PR.

@alanhamlett alanhamlett merged commit 283b280 into wtforms:master Jul 22, 2018
1 check passed
@alanhamlett alanhamlett deleted the ahamlett-email_validation_using_library branch Jul 22, 2018
azmeuk added a commit to azmeuk/wtforms that referenced this issue Apr 20, 2020
quis added a commit to alphagov/notifications-admin that referenced this issue Apr 22, 2020
WTForms requires this dependency now. This was changed back in June
2018 (wtforms/wtforms#429) but has only just
been released now.
quis added a commit to alphagov/notifications-admin that referenced this issue Apr 23, 2020
This involves three changes which broke our code.

To validate email addresses, the optional dependency `email-validator`
must be installed<sup>1</sup>. But since we don’t use WTForms’ email
validation, we shouldn’t need to do this. Instead this commit makes our
own `EmailField`, which just handles rendering the field as an email
field, not validating that (because we add the validation elsewhere).

When rendering textareas, and extra `\r\n` is inserted at the beginning
<sup>2</sup>. Browsers will strip this when displaying the textbox and
submitting the form, but some of our tests need updating to account for
this.

The error message for when you don’t choose an option from some radio
buttons has now changed. Rather than just accepting WTForms’ new
message, this commit makes the error messages like the examples from
the Design System<sup>3</sup>. By default it will say ‘Select an
option’, but by passing in an extra parameter (`thing`) it can be
customised to be more specific, for example ‘Select a type of
organisation’.

***

1. wtforms/wtforms#429
2. wtforms/wtforms#238
3. https://design-system.service.gov.uk/components/radios/#error-messages
quis added a commit to alphagov/notifications-admin that referenced this issue Apr 23, 2020
This involves three changes which broke our code.

To validate email addresses, the optional dependency `email-validator`
must be installed<sup>1</sup>. But since we don’t use WTForms’ email
validation, we shouldn’t need to do this. Instead this commit makes our
own `EmailField`, which just handles rendering the field as an email
field, not validating that (because we add the validation elsewhere).

When rendering textareas, and extra `\r\n` is inserted at the beginning
<sup>2</sup>. Browsers will strip this when displaying the textbox and
submitting the form, but some of our tests need updating to account for
this.

The error message for when you don’t choose an option from some radio
buttons has now changed. Rather than just accepting WTForms’ new
message, this commit makes the error messages like the examples from
the Design System<sup>3</sup>. By default it will say ‘Select an
option’, but by passing in an extra parameter (`thing`) it can be
customised to be more specific, for example ‘Select a type of
organisation’.

***

1. wtforms/wtforms#429
2. wtforms/wtforms#238
3. https://design-system.service.gov.uk/components/radios/#error-messages
quis added a commit to alphagov/notifications-admin that referenced this issue Apr 23, 2020
This involves three changes which broke our code.

To validate email addresses, the optional dependency `email-validator`
must be installed<sup>1</sup>. But since we don’t use WTForms’ email
validation, we shouldn’t need to subclass it – it can just be its own
self contained thing. Then we don’t need to add the extra dependency.

When rendering textareas, and extra `\r\n` is inserted at the beginning
<sup>2</sup>. Browsers will strip this when displaying the textbox and
submitting the form, but some of our tests need updating to account for
this.

The error message for when you don’t choose an option from some radio
buttons has now changed. Rather than just accepting WTForms’ new
message, this commit makes the error messages like the examples from
the Design System<sup>3</sup>. By default it will say ‘Select an
option’, but by passing in an extra parameter (`thing`) it can be
customised to be more specific, for example ‘Select a type of
organisation’.

***

1. wtforms/wtforms#429
2. wtforms/wtforms#238
3. https://design-system.service.gov.uk/components/radios/#error-messages
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants