Proposal to let the caller analyse the reason for ambiguous verification results #55
No reviewers
Labels
No milestone
No project
No assignees
1 participant
Notifications
Due date
No due date set.
Dependencies
No dependencies set.
Reference: karolyi/py3-validate-email#55
Loading…
Reference in a new issue
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?
This is my proposal for allowing the caller of
validate_email_or_failto analyze the reasons for ambiguous verification results and, if desired, react appropriately.It adds two new parameters:
raise_communication_errorscan be set toTrueto raise anSMTPCommunicationErrorexception in case we don't even get to theRCPT TOstep in the SMTP communication. The exception contains all information to analyze the exact error code and message. If the parameter is set toFalse(the default), such a case is considered an ambiguous verification result, and the check function returnsNone.raise_temporary_errorscan be set toTrueto raise anSMTPTemporaryErrorexception in case theRCPT TOcommand is answered with a 4xx status code. The exception contains all information to analyze the exact error code and message. If the parameter is set toFalse(the default), such a case is considered an ambiguous verification result, and the check function returnsNone.Comments, bug reports, and suggestions for improvements are welcome!
I am not sure you need this much changes, especially with the added new parameters. You also unified functions that were intentionally kept separate for clarity.
All in all, I like the new exceptions, but I can't help but feel you overengineered it.
@ -172,4 +58,3 @@The package contains an auto-updater for downloading and updating the built-in blacklist.txt. It will run on each module load (and installation), but will try to update the content only if the file is older than 5 days, and if the content is not the same that's already downloaded.The update can be triggered manually::I am not sure you need this. The function returning
True/False/Nonedoesn't need this, and in case of the function that raises errors, you should just simply raise the ambigious exceptions. No extra parameters needed.@ -0,0 +75,4 @@self, local_hostname: str, timeout: float, debug: bool,raise_communication_errors: bool,raise_temporary_errors: bool,sender: str, recip: str):Do you handle 3xx responses here? Not sure if it doesn't need to be an error, but since they're not handled, I'd keep the original here.
@ -0,0 +92,4 @@# Avoid error on close() after unsuccessful connectself.sock = Nonedef putcmd(self, cmd, args=""):See above ...
@ -0,0 +272,4 @@local_hostname=helo_host, timeout=smtp_timeout, debug=debug,raise_communication_errors=raise_communication_errors,raise_temporary_errors=raise_temporary_errors,sender=from_address.ace, recip=email_address.ace)The reason why I had this function is to keep the functions smaller than ~24 lines per function. If you end up with a huge function, it is much harder to understand how it works. Can you revert this with your added changes?
@ -0,0 +75,4 @@self, local_hostname: str, timeout: float, debug: bool,raise_communication_errors: bool,raise_temporary_errors: bool,sender: str, recip: str):Responses 2xx and 3xx generally indicate success, only 4xx and 5xx indicate failure. Actually at this point in the communication, I don't think if any 3xx response is even possible, but if there is one, it would not indicate an error.
@ -0,0 +75,4 @@self, local_hostname: str, timeout: float, debug: bool,raise_communication_errors: bool,raise_temporary_errors: bool,sender: str, recip: str):3yz (Positive Intermediate Reply): The command has been accepted, but the requested action is being held in abeyance, pending receipt of further information.Alright, I'll resolve this part then.
@ -0,0 +272,4 @@local_hostname=helo_host, timeout=smtp_timeout, debug=debug,raise_communication_errors=raise_communication_errors,raise_temporary_errors=raise_temporary_errors,sender=from_address.ace, recip=email_address.ace)Splitting this into two functions would mean that I have to pass a lot of things back and forth, for example I'd have to pass both error collection variables into _check_one_mx so they can be filled there. I first tried it that way, but I had the impression that the code is actually harder to understand with split functions. However, if you think it's better, I can of course change it.
@ -172,4 +58,3 @@The package contains an auto-updater for downloading and updating the built-in blacklist.txt. It will run on each module load (and installation), but will try to update the content only if the file is older than 5 days, and if the content is not the same that's already downloaded.The update can be triggered manually::The idea behind this is to keep the default behavior unchanged for verify_email_or_fail for all the users that already use that function and rely on the fact that for ambiguous cases it doesn't raise an exception.
@ -0,0 +272,4 @@local_hostname=helo_host, timeout=smtp_timeout, debug=debug,raise_communication_errors=raise_communication_errors,raise_temporary_errors=raise_temporary_errors,sender=from_address.ace, recip=email_address.ace)Sounds like a candidate for an entire class with methods for me then ... If it gets this complicated, classes provide a better way for communication between the smaller parts.
@ -172,4 +58,3 @@The package contains an auto-updater for downloading and updating the built-in blacklist.txt. It will run on each module load (and installation), but will try to update the content only if the file is older than 5 days, and if the content is not the same that's already downloaded.The update can be triggered manually::Ah yes, that problem. I think since we're already heading towards a major release with breaking changes, might as well restructure the code to fit the new purpose and then release it as such.
@ -0,0 +272,4 @@local_hostname=helo_host, timeout=smtp_timeout, debug=debug,raise_communication_errors=raise_communication_errors,raise_temporary_errors=raise_temporary_errors,sender=from_address.ace, recip=email_address.ace)Interesting idea, I think I'll give it a try. (Not today, though)
@ -0,0 +272,4 @@local_hostname=helo_host, timeout=smtp_timeout, debug=debug,raise_communication_errors=raise_communication_errors,raise_temporary_errors=raise_temporary_errors,sender=from_address.ace, recip=email_address.ace)Take your time :) I'm not Mr. Know It All, these are just my suggestions according to my experiences :) I always try to keep it clean and simple, so that anybody who comes into the project can easily contribute.
Please have a look at the updated PR. Following your suggestion to run the SMTP test in a class with methods, I realized that it can nicely be implemented as a subclass of the standard
smtplib.SMTPclass. I hope I managed to make the code readable and understandable.There is a unittest that now fails, I would need some help how to update it to the new implementation.
For now I kept the parameters to switch on and off the extra exceptions on communication issues or ambiguous responses. When thinking it over, I felt that it should be the default that in these cases there are no exceptions, but users should be add them if they want to investigate deeper. Also, this way, the current PR should not break existing code.
I've cleaned the code up quite a bit starting from your changes and had to create a separate branch for it :https://github.com/karolyi/py3-validate-email/tree/pull-55
Closing this here, the changes have to be made there. The code seems to be working for me now, but I have to add more detailed tests so that the functionality is tested thoroughly.
Pull request closed