karolyi
/
py3-validate-email
2
2
Fork 4
Code Issues 1 Pull Requests Projects Releases Wiki Activity

Introduce "EmailAddress" class #11

Merged
reinhard-mueller merged 1 commits from master into master 2020-04-13 10:40:08 +02:00
Conversation 10 Commits 1 Files Changed 11 +190 -145
reinhard-mueller commented 2020-04-12 13:37:32 +02:00 (Migrated from github.com)
Copy Link

This introduces a class that unifies the tasks of splitting email address into user and domain part as well as converting an international domain name into the ASCII-compatible encoding.

@karolyi I have tried to follow the general coding style of the project.

Please tell me if you see room for improvement.

This introduces a class that unifies the tasks of splitting email address into user and domain part as well as converting an international domain name into the ASCII-compatible encoding. @karolyi I have tried to follow the general coding style of the project. Please tell me if you see room for improvement.
karolyi (Migrated from github.com) reviewed 2020-04-12 14:04:07 +02:00
validate_email/email_address.py
karolyi (Migrated from github.com) commented 2020-04-12 14:04:06 +02:00
Copy Link
    """
    Internally used class to hold an email address.

    This class featuers splitting the email address into user and domain
    part as well as converting internationalized domain name into the
    ASCII-compatible encoding (ACE) according to the IDNA standard.
    """
```suggestion """ Internally used class to hold an email address. This class featuers splitting the email address into user and domain part as well as converting internationalized domain name into the ASCII-compatible encoding (ACE) according to the IDNA standard. """ ```
karolyi (Migrated from github.com) reviewed 2020-04-12 14:04:33 +02:00
validate_email/email_address.py
karolyi (Migrated from github.com) commented 2020-04-12 14:04:33 +02:00
Copy Link

As per PEP8, docstrings terminate at 72 columns.

As per PEP8, docstrings terminate at 72 columns.
karolyi commented 2020-04-12 14:05:26 +02:00 (Migrated from github.com)
Copy Link

can you please pull my latest changes on master?

can you please pull my latest changes on master?
karolyi (Migrated from github.com) reviewed 2020-04-12 14:15:15 +02:00
karolyi (Migrated from github.com) left a comment
Copy Link

I see you also removed tests for the email format checks.

Please add those back for testing your new class, the tests are a good quality indicator.

I see you also removed tests for the email format checks. Please add those back for testing your new class, the tests are a good quality indicator.
validate_email/email_address.py
karolyi (Migrated from github.com) commented 2020-04-12 14:06:58 +02:00
Copy Link
        """
        The username part of the email address, that is the part before
        the "@" sign.
        """

same here with the PEP8 stuff

```suggestion """ The username part of the email address, that is the part before the "@" sign. """ ``` same here with the PEP8 stuff
karolyi (Migrated from github.com) commented 2020-04-12 14:08:01 +02:00
Copy Link
        """
        The domain part of the email address, that is the part after the
        "@" sign.
        """
```suggestion """ The domain part of the email address, that is the part after the "@" sign. """ ```
karolyi (Migrated from github.com) commented 2020-04-12 14:08:37 +02:00
Copy Link
        """
        The ASCII-compatible encoding for the domain part of the email
        address.
        """
```suggestion """ The ASCII-compatible encoding for the domain part of the email address. """ ```
reinhard-mueller commented 2020-04-12 14:17:57 +02:00 (Migrated from github.com)
Copy Link

Will add the test soon, must eat some cake first ;-)

Will add the test soon, must eat some cake first ;-)
karolyi commented 2020-04-12 15:25:40 +02:00 (Migrated from github.com)
Copy Link

I'm still fighting with the installer part (python package installers are a mess), so bare with me until I get to review your changes.

I'm still fighting with the installer part (python package installers are a mess), so bare with me until I get to review your changes.
reinhard-mueller commented 2020-04-12 15:32:06 +02:00 (Migrated from github.com)
Copy Link

@karolyi No worries, I'm perfectly fine enjoying the Easter Sunday sun meanwhile :-)

P.S. all your comments on my changes are much appreciated, please don't hesitate to point me to any possible improvement!

@karolyi No worries, I'm perfectly fine enjoying the Easter Sunday sun meanwhile :-) P.S. all your comments on my changes are much appreciated, please don't hesitate to point me to any possible improvement!
karolyi (Migrated from github.com) reviewed 2020-04-12 16:52:31 +02:00
validate_email/constants.py
@ -6,4 +6,3 @@
r'((?:[A-Z0-9](?:[A-Z0-9-]{0,61}[A-Z0-9])?\.)+)'
r'(?:[A-Z0-9-]{2,63}(?<!-))\Z', IGNORECASE)
EMAIL_EXTRACT_HOST_REGEX = re_compile(r'(?<=@)\[?([^\[\]]+)')
LITERAL_REGEX = re_compile(
karolyi (Migrated from github.com) commented 2020-04-12 16:48:28 +02:00
Copy Link

I would keep this constant and use for dissecting email address user and domain parts, so the problem you mentioned in the other issue will be easier to address.

I would keep this constant and use for dissecting email address user and domain parts, so the problem you mentioned in the other issue will be easier to address.
validate_email/regex_check.py
@ -58,2 +45,4 @@
ip_address = literal_match.group(1)
if _validate_ipv46_address(ip_address):
return True
karolyi (Migrated from github.com) commented 2020-04-12 16:52:27 +02:00
Copy Link

What was the reason for removing RegexValidator? I don't see it, maybe you could explain?

What was the reason for removing RegexValidator? I don't see it, maybe you could explain?
karolyi (Migrated from github.com) reviewed 2020-04-12 16:56:10 +02:00
validate_email/email_address.py
karolyi (Migrated from github.com) commented 2020-04-12 16:56:10 +02:00
Copy Link
class EmailAddress(object):

python3 object

```suggestion class EmailAddress(object): ``` python3 object
karolyi (Migrated from github.com) reviewed 2020-04-12 16:56:54 +02:00
validate_email/email_address.py
karolyi (Migrated from github.com) commented 2020-04-12 16:56:53 +02:00
Copy Link
    def user(self) -> str:
```suggestion def user(self) -> str: ```
karolyi (Migrated from github.com) reviewed 2020-04-12 16:57:04 +02:00
validate_email/email_address.py
karolyi (Migrated from github.com) commented 2020-04-12 16:57:04 +02:00
Copy Link
    def domain(self) -> str:
```suggestion def domain(self) -> str: ```
karolyi (Migrated from github.com) reviewed 2020-04-12 16:57:17 +02:00
validate_email/email_address.py
karolyi (Migrated from github.com) commented 2020-04-12 16:57:17 +02:00
Copy Link
    def ace(self) -> str:
```suggestion def ace(self) -> str: ```
karolyi (Migrated from github.com) reviewed 2020-04-12 16:57:27 +02:00
validate_email/email_address.py
karolyi (Migrated from github.com) commented 2020-04-12 16:57:27 +02:00
Copy Link
    def ace_domain(self) -> str:
```suggestion def ace_domain(self) -> str: ```
karolyi (Migrated from github.com) reviewed 2020-04-12 17:01:02 +02:00
validate_email/exceptions.py
karolyi (Migrated from github.com) commented 2020-04-12 17:01:02 +02:00
Copy Link
    """
    Raised when the from email address used for the MX check has an
    invalid format.
    """
```suggestion """ Raised when the from email address used for the MX check has an invalid format. """ ```
karolyi (Migrated from github.com) requested changes 2020-04-12 17:06:58 +02:00
karolyi (Migrated from github.com) left a comment
Copy Link

Looking good so far, other than the two things I asked/questioned.
If you change/explain those, a squash on the commits will be necessary, and then I think we'll be good to go.

Looking good so far, other than the two things I asked/questioned. If you change/explain those, a squash on the commits will be necessary, and then I think we'll be good to go.
reinhard-mueller (Migrated from github.com) reviewed 2020-04-12 17:19:19 +02:00
validate_email/constants.py
@ -6,4 +6,3 @@
r'((?:[A-Z0-9](?:[A-Z0-9-]{0,61}[A-Z0-9])?\.)+)'
r'(?:[A-Z0-9-]{2,63}(?<!-))\Z', IGNORECASE)
EMAIL_EXTRACT_HOST_REGEX = re_compile(r'(?<=@)\[?([^\[\]]+)')
LITERAL_REGEX = re_compile(
reinhard-mueller (Migrated from github.com) commented 2020-04-12 17:19:19 +02:00
Copy Link

If I am not mistaken, if an IP address is used as the domain part of an email address, it must be enclosed in brackets. So syntactically, user@[123.123.123.123] is a valid email address, while user@123.123.123.123 isn't (unless 123.123.123.123 can be considered a domain name). This regex removes the brackets, so we will later not be able to distinguish between these cases. I think it's better to just use the rsplit and keep the brackets.

If I am not mistaken, if an IP address is used as the domain part of an email address, it **must** be enclosed in brackets. So syntactically, user@[123.123.123.123] is a valid email address, while user@123.123.123.123 isn't (unless 123.123.123.123 can be considered a domain name). This regex *removes* the brackets, so we will later not be able to distinguish between these cases. I think it's better to just use the rsplit and keep the brackets.
reinhard-mueller (Migrated from github.com) reviewed 2020-04-12 17:21:12 +02:00
validate_email/regex_check.py
@ -58,2 +45,4 @@
ip_address = literal_match.group(1)
if _validate_ipv46_address(ip_address):
return True
reinhard-mueller (Migrated from github.com) commented 2020-04-12 17:21:12 +02:00
Copy Link

The reason was simply that the class has degenerated to containing only a call() method and nothing else, so it can be replaced by a plain function. Is there any reason to wrap this function into a class I didn't see?

The reason was simply that the class has degenerated to containing only a __call__() method and nothing else, so it can be replaced by a plain function. Is there any reason to wrap this function into a class I didn't see?
reinhard-mueller commented 2020-04-12 17:22:56 +02:00 (Migrated from github.com)
Copy Link

I've posted my considerations on your questions. I totally agree on the squash commit :-)

I've posted my considerations on your questions. I totally agree on the squash commit :-)
karolyi (Migrated from github.com) approved these changes 2020-04-12 18:55:18 +02:00
karolyi commented 2020-04-12 18:56:17 +02:00 (Migrated from github.com)
Copy Link

Alright, LGTM.

Please squash your commit into one and I'll merge the PR and make a release.

Again, thank you for your contribution! Much appreciated.

Alright, LGTM. Please squash your commit into one and I'll merge the PR and make a release. Again, thank you for your contribution! Much appreciated.
reinhard-mueller commented 2020-04-13 09:35:03 +02:00 (Migrated from github.com)
Copy Link

Sorry @karolyi I think I need your help with squashing the commits into one. Can I do this within this pull request and with in this branch, or do I have to create a new branch and a new pull request?

P.S. I thought that squashing into one is something that you do when you apply the pull request, so I am somwhat lost here.

Sorry @karolyi I think I need your help with squashing the commits into one. Can I do this within this pull request and with in this branch, or do I have to create a new branch and a new pull request? P.S. I thought that squashing into one is something that you do when you apply the pull request, so I am somwhat lost here.
reinhard-mueller commented 2020-04-13 09:48:06 +02:00 (Migrated from github.com)
Copy Link

@karolyi please ignore my previous message, I managed to do it. Learned something new about git today :-)

I think you should be fine to pull now. Thank you for your cooperation in this!

@karolyi please ignore my previous message, I managed to do it. Learned something new about git today :-) I think you should be fine to pull now. Thank you for your cooperation in this!
reinhard-mueller commented 2020-04-13 09:52:25 +02:00 (Migrated from github.com)
Copy Link

BTW, I think it's not really necessary to make a release because of this change, because it doesn't add a new user-visisble feature.

BTW, I think it's not really necessary to make a release because of this change, because it doesn't add a new user-visisble feature.
karolyi commented 2020-04-13 10:58:04 +02:00 (Migrated from github.com)
Copy Link

Thank you!

I'll issue a release nevertheless, just to make sure everybody has your changes. If there's any problems with it, new issues will be posted.

Also, I have some smaller changes, that too should be released.

Thank you! I'll issue a release nevertheless, just to make sure everybody has your changes. If there's any problems with it, new issues will be posted. Also, I have some smaller changes, that too should be released.
Sign in to join this conversation.
Reviewers
No reviewers
karolyi
Labels
Clear labels   
bug

Something isn't working

  
duplicate

This issue or pull request already exists

  
enhancement

New feature or request

  
good first issue

Good for newcomers

  
help wanted

Extra attention is needed

  
invalid

This doesn't seem right

  
question

Further information is requested

  
stale

   
wontfix

This will not be worked on

No Label
bug
duplicate
enhancement
good first issue
help wanted
invalid
question
stale
wontfix
Milestone
Clear milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
No Assignees
1 Participants
Notifications
Due Date
The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: karolyi/py3-validate-email#11
No description provided.
Delete Branch "master"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?