karolyi/py3-validate-email

Fork 0

Clean up before major release #58

Merged

reinhard-mueller merged 3 commits from master into pull-55

2021-03-07 12:59:25 +01:00

reinhard-mueller commented

2021-03-02 18:39:11 +01:00

(Migrated from github.com)

While working on the cleanup of the public interface we've discussed, I stumbled over two additional changes which I think would make the upcoming PR more logical and the structure more consistent. @karolyi please let me know what you think about these changes. I've purposedly split them in two separate commits which you can look at independently. If you agree with what I did, I'll continue with the parameter list cleanup in this PR.

reinhard-mueller commented

2021-03-06 18:22:03 +01:00

(Migrated from github.com)

@karolyi I would welcome your comments on the commits I already did before continuing my work on this pull request.

karolyi commented

2021-03-06 18:23:05 +01:00

(Migrated from github.com)

Its not forgotten, I'm just busy... maybe tomorrow.

reinhard-mueller commented

2021-03-06 18:37:08 +01:00

(Migrated from github.com)

No rush, I'm quite busy myself as well anyway.

karolyi (Migrated from github.com) requested changes 2021-03-07 11:28:14 +01:00

karolyi (Migrated from github.com) left a comment

There are a couple changes (some only formal) that needs to be done, and some more tests for the _SMTPChecker object to be created.

Other than that, it's LGTM. Once I merged it with your changes, I'll give it a go in my editor to see if it's got any weirdness I can see, but from what I can see from looking at the PR in the browser, it's generally good to go.

There are a couple changes (some only formal) that needs to be done, and some more tests for the _SMTPChecker object to be created. Other than that, it's LGTM. Once I merged it with your changes, I'll give it a go in my editor to see if it's got any weirdness I can see, but from what I can see from looking at the PR in the browser, it's generally good to go.

validate_email/smtp_check.py

karolyi (Migrated from github.com) commented

2021-03-07 11:23:17 +01:00

Why the wrapping change here? As per PEP8, comment lines should be <72 columns long, this is why it was linebroken at before 'determined'.

validate_email/validate_email.py

karolyi (Migrated from github.com) commented

2021-03-07 11:21:51 +01:00

I'd keep the warnings in the case of the debug flag set. Users were complaining about them being unable to turn off logging before.

validate_email/validate_email.py

karolyi (Migrated from github.com) commented

2021-03-07 11:22:05 +01:00

Same as above.

karolyi (Migrated from github.com) reviewed 2021-03-07 11:33:32 +01:00

validate_email/validate_email.py

karolyi (Migrated from github.com) commented

2021-03-07 11:33:31 +01:00

See #30

reinhard-mueller (Migrated from github.com) reviewed 2021-03-07 11:43:19 +01:00

validate_email/validate_email.py

reinhard-mueller (Migrated from github.com) commented

2021-03-07 11:43:19 +01:00

Just to make sure: have you seen my comment in commit a0f1cd1b04, and do you disagree with it? In short, my reasoning for the change was:

Info fits better than warning, because it's expected library behaviour in a standard usecase
Info is not output by default, so everybody not configuring logging explicitly will get no logging output
It makes it possible to enable our logging messages without enabling the very verbose and non-configurable smtplib debug output.

I have no problem reverting this change, but I want to make sure you really disagree with my reasons.

Just to make sure: have you seen my comment in commit a0f1cd1b046cb76aef052246db5f4ce750c00f86, and do you disagree with it? In short, my reasoning for the change was: * Info fits better than warning, because it's expected library behaviour in a standard usecase * Info is not output by default, so everybody not configuring logging explicitly will get no logging output * It makes it possible to enable our logging messages without enabling the very verbose and non-configurable smtplib debug output. I have no problem reverting this change, but I want to make sure you really disagree with my reasons.

reinhard-mueller (Migrated from github.com) reviewed 2021-03-07 11:45:02 +01:00

validate_email/smtp_check.py

reinhard-mueller (Migrated from github.com) commented

2021-03-07 11:45:02 +01:00

Sorry, I misunderstood the rule, I thought it was <= 72 columns. I can change this back, but then the next line would have to be broken before the "for", right?

karolyi (Migrated from github.com) reviewed 2021-03-07 12:42:33 +01:00

validate_email/validate_email.py

karolyi (Migrated from github.com) commented

2021-03-07 12:42:33 +01:00

You have a point there. I remember somebody complaning seeing log messages coming from this module when he used it with a command line tool he built. I can't find the issue now, but I remember having it posted. That said AFAIK info doesn't get logged to the stderr so I'll mark this as resolved and leave it as is.

You have a point there. I remember somebody complaning seeing log messages coming from this module when he used it with a command line tool he built. I can't find the issue now, but I remember having it posted. That said AFAIK `info` doesn't get logged to the stderr so I'll mark this as resolved and leave it as is.

karolyi (Migrated from github.com) reviewed 2021-03-07 12:52:42 +01:00

validate_email/smtp_check.py

karolyi (Migrated from github.com) commented

2021-03-07 12:52:42 +01:00

Quoting PEP8:

The Python standard library is conservative and requires limiting lines to 79 characters (and docstrings/comments to 72).

Here's how it looks in my editor (I have a ruler set to 72 and 79 colums (I use Sublime Text):

Quoting PEP8: > The Python standard library is conservative and requires limiting lines to 79 characters (and docstrings/comments to 72). Here's how it looks in my editor (I have a ruler set to 72 and 79 colums (I use Sublime Text): ![2021-03-07 12_52_15-~_Work_opensource_py3-validate-email_validate_email_smtp_check py (py3-validate-](https://user-images.githubusercontent.com/987055/110238814-9ab0ad00-7f3b-11eb-91e7-6f1d1ff35b63.png)

karolyi (Migrated from github.com) reviewed 2021-03-07 12:58:11 +01:00

validate_email/smtp_check.py

karolyi (Migrated from github.com) commented

2021-03-07 12:58:11 +01:00

Ok it's my bad, it seems it actually looks good, I got a bit confused here by looking at the webpage based diff. This is why I like to look at stuff in my editor, as you can see. :)