Skip to content

Correctly handle None, add tests for other invalid types #155

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lightswitch05
Copy link

@lightswitch05 lightswitch05 commented Apr 21, 2025

Why I made this PR

Hello! It seems calling .decode on None raises an AttributeError - which is unfortunately not handled by this library. Calling validate_email(None) generates this exception:

AttributeError: 'NoneType' object has no attribute 'decode'

What is included in the PR

I've added a catch for the AttributeError and some additional tests for invalid types.

Thank you for maintaining this library!

@JoshData
Copy link
Owner

Hi. Thanks.

But there are two reasons I can't accept this as-is:

  1. Handling the incorrect argument type by raising an EmailSyntaxError isn't really right. It should probably be a TypeError for an argument that is not a str or bytes.
  2. Using the AttributeError to detect an incorrect type is also not really great. I would prefer just explicitly checking if the argument is str or bytes, and then if not raise a TypeError.

@lightswitch05
Copy link
Author

Hey @JoshData - I'm happy to make any required adjustments. My original intent was to keep the changes as minimal as possible - which lead me to this solution. I agree that explicitly checking is the better way to go about this. I also agree that EmailSyntaxError isn't a great fit either. Before I make any changes, I would like to gather some requirements. Specifically, I would like to dig into the TypeError suggestion.

From the perspective of a user of this library. I have a value and I want to use this tool to verify that value is a email address. Per the docs 'Quick Start' , I've setup a try/except block with EmailNotValidError and ship it to production. Of course, None shows up. Since it throws TypeError instead of EmailNotValidError - which I've not handled - my application crashes.

Just from my point of view, its unexpected behavior that I need to do some validation on the potential email address before handing it off to the library to the rest of the email validation. Perhaps a new exception can be created that extends EmailNotValidError? Perhaps EmailNotValidTypeError(EmailNotValidError)? That way, everyone catching EmailNotValidError remains fully-covered from an unhanded exception point-of-view, but for people needing to know why it failed, there is a more specific exception type that can be used to make that determination?

How does that sound? Do you have any better naming ideas instead of EmailNotValidTypeError?

@JoshData
Copy link
Owner

I need to do some validation on the potential email address before handing it off to the library

I don't think that's the right way of looking at this case. If you don't use a function according to its documentation, a TypeError or ValueError is common and is usually a good thing because it tells you there is an error in your code that probably should be fixed.

@lightswitch05
Copy link
Author

Thanks for the update @JoshData. I think I'm getting a clearer picture of expectations now. So, in the case that bytes are provided, but they are not ascii, I'm thinking that example should be updated to raise a ValueError as well? It seems like that doesn't really fit in well with EmailSyntaxError either?

if not isinstance(email, str):
try:
email = email.decode("ascii")
except ValueError as e:
raise EmailSyntaxError("The email address is not valid ASCII.") from e

@JoshData
Copy link
Owner

That makes sense. That code block is very old and not documented so I wasn't really thinking about that part carefully. Updating the exception is a good idea.

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.

2 participants