Introduce "EmailAddress" class #11

Merged
reinhard-mueller merged 1 commit from master into master 2020-04-13 10:40:08 +02:00
reinhard-mueller commented 2020-04-12 13:37:32 +02:00 (Migrated from github.com)

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
karolyi (Migrated from github.com) commented 2020-04-12 14:04:06 +02:00
    """
    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
karolyi (Migrated from github.com) commented 2020-04-12 14:04:33 +02:00

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)

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

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.
karolyi (Migrated from github.com) commented 2020-04-12 14:06:58 +02:00
        """
        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
        """
        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
        """
        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)

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)

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)

@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
@ -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

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.
@ -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

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
karolyi (Migrated from github.com) commented 2020-04-12 16:56:10 +02:00
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
karolyi (Migrated from github.com) commented 2020-04-12 16:56:53 +02:00
    def user(self) -> str:
```suggestion def user(self) -> str: ```
karolyi (Migrated from github.com) reviewed 2020-04-12 16:57:04 +02:00
karolyi (Migrated from github.com) commented 2020-04-12 16:57:04 +02:00
    def domain(self) -> str:
```suggestion def domain(self) -> str: ```
karolyi (Migrated from github.com) reviewed 2020-04-12 16:57:17 +02:00
karolyi (Migrated from github.com) commented 2020-04-12 16:57:17 +02:00
    def ace(self) -> str:
```suggestion def ace(self) -> str: ```
karolyi (Migrated from github.com) reviewed 2020-04-12 16:57:27 +02:00
karolyi (Migrated from github.com) commented 2020-04-12 16:57:27 +02:00
    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
karolyi (Migrated from github.com) commented 2020-04-12 17:01:02 +02:00
    """
    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

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
@ -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

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
@ -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

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)

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)

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)

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)

@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)

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)

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.
No reviewers
No milestone
No project
No assignees
1 participant
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.