Improve error reporting #48

2021-02-09T23:48:59+01:00

reinhard-mueller commented

2021-02-09 23:48:59 +01:00

(Migrated from github.com)

Unify error message format, always include SMTP status code
Use "with SMTP" to properly close connection even in case of error
Establish connection after setting debug level so this is debugged as
well
Properly handle SMTP error status codes on connection
Don't return ambigious SMTP status codes, as they are not used anyway

@karolyi @manojr2k I tried with some domains, but I think it would be good if you could do some more checks with this code before doing a release.

* Unify error message format, always include SMTP status code * Use "with SMTP" to properly close connection even in case of error * Establish connection after setting debug level so this is debugged as well * Properly handle SMTP error status codes on connection * Don't return ambigious SMTP status codes, as they are not used anyway @karolyi @manojr2k I tried with some domains, but I think it would be good if you could do some more checks with this code before doing a release.

karolyi (Migrated from github.com) requested changes 2021-02-10 16:14:01 +01:00

karolyi (Migrated from github.com) left a comment

I see a problem with this PR. Before, if there was a 4XX response code, the messages were added to the result. Now, that entire part is removed, and the 4XX response with the message completely suppressed. That's a breaking change. Could you add that part back?

reinhard-mueller commented

2021-02-10 19:59:16 +01:00

(Migrated from github.com)

Thank you for the hint about the travis error, I fixed that.

I see a problem with this PR. Before, if there was a 4XX response code, the messages were added to the result. Now, that entire part is removed, and the 4XX response with the message completely suppressed. That's a breaking change. Could you add that part back?

To me it looks like in the current code, the 4XX response codes returned from _smtp_converse are indeed added to the error_messages list in _check_one_mx, but at the same time the latter function returns True, so the error message list is not used. That's why I removed that part of the code: at first sight it looks as if the 4XX response codes were collected, but only when looking closer you see that they are discarded again later, so it's quite confusing.

Thank you for the hint about the travis error, I fixed that. > I see a problem with this PR. Before, if there was a 4XX response code, the messages were added to the result. Now, that entire part is removed, and the 4XX response with the message completely suppressed. That's a breaking change. Could you add that part back? To me it looks like in the current code, the 4XX response codes returned from `_smtp_converse` are indeed added to the `error_messages` list in `_check_one_mx`, but at the same time the latter function returns `True`, so the error message list is not used. That's why I removed that part of the code: at first sight it looks as if the 4XX response codes were collected, but only when looking closer you see that they are discarded again later, so it's quite confusing.

karolyi commented

2021-02-11 11:37:28 +01:00

(Migrated from github.com)

Yeah, I see what you mean now. I think it would be a useful information to keep it somehow for the user, but with the current structure it's not really available.

I'll merge this now and we'll take care of the collecting of ambiguous messages later, when necessary. Thanks!

Yeah, I see what you mean now. I think it would be a useful information to keep it somehow for the user, but with the current structure it's not really available. I'll merge this now and we'll take care of the collecting of ambiguous messages later, when necessary. Thanks!

Sign in to join this conversation.

No reviewers

karolyi