mx_check() function doesn't log exceptions #3

New Issue

2020-02-28T17:23:43+01:00

agouil commented

2020-02-28 17:23:43 +01:00

(Migrated from github.com)

The mx_check() function inside mx_check.py catches all ValueError exceptions and it just returns False with no additional information on what the error is.

I was having some trouble validating a hotmail.co.uk email address (the address exists and is valid) and as I was tracing the error, I realised the above.

Is there a way to add logging to the mx_check() function when handling the ValueError so there is visibility on the actual errors which caused the validation to fail?

The `mx_check()` function inside `mx_check.py` catches all `ValueError` exceptions and it just returns `False` with no additional information on what the error is. I was having some trouble validating a hotmail.co.uk email address (_the address exists and is valid_) and as I was tracing the error, I realised the above. Is there a way to add logging to the `mx_check()` function when handling the `ValueError` so there is visibility on the actual errors which caused the validation to fail?

karolyi commented

2020-02-28 17:32:46 +01:00

(Migrated from github.com)

Hey,

thanks for the report. I could let the ValueError bubble up in the stack, but that would kinda beat the purpose of returning a true/false/none which would simply tell the developer if the address is usable or not.

Can you tell me what the cause for the ValueError was in your case?

Hey, thanks for the report. I could let the `ValueError` bubble up in the stack, but that would kinda beat the purpose of returning a true/false/none which would simply tell the developer if the address is usable or not. Can you tell me what the cause for the `ValueError` was in your case?

agouil commented

2020-02-28 18:28:29 +01:00

(Migrated from github.com)

Looking more into it there wasn't a ValueError after all. It was an inconclusive result, so function returned None. Anyways, my case is resolved, I think for now.

I totally agree the function should return true/false/none. However, I was thinking something like the below:

try:
    mx_records = _get_mx_records(domain=domain, timeout=dns_timeout)
except ValueError as e:
    logger.exception(e)
    return False

Then, it's up to each developer to stream these log messages to stderr - especially, if someone is using the module inside a server environment. In this case, they can choose which loggers to collect messages from.

Looking more into it there wasn't a `ValueError` after all. It was an inconclusive result, so function returned `None`. Anyways, my case is resolved, I think for now. I totally agree the function should return true/false/none. However, I was thinking something like the below: ``` try: mx_records = _get_mx_records(domain=domain, timeout=dns_timeout) except ValueError as e: logger.exception(e) return False ``` Then, it's up to each developer to stream these log messages to stderr - especially, if someone is using the module inside a server environment. In this case, they can choose which loggers to collect messages from.

reinhard-mueller commented

2020-04-08 08:47:12 +02:00

(Migrated from github.com)

I think this is a very good idea, and I would go even further and say that any reason why an email address is refused should be logged. Also, I'd log with logger.info because the traceback of the exception is not interesting, and a log level of ERROR does not fit an event that occurs in regular use of the library.

I think this is a very good idea, and I would go even further and say that *any* reason why an email address is refused should be logged. Also, I'd log with `logger.info` because the traceback of the exception is not interesting, and a log level of `ERROR` does not fit an event that occurs in regular use of the library.

reinhard-mueller commented

2020-04-08 12:37:22 +02:00

(Migrated from github.com)

Thinking this further, I'd like to propose that:

Each possible reason for the validation to fail raises a specific exception, each of them subclassed from a EmailValidationError.
A new public function check_email() does all the checks, letting the exceptions through to the caller so the caller can decide what to do with that information, like even displaying a detailled error message in the user interface.
The existing function validate_email() just calls check_email(), catches all EmailValidationError exceptions, logs them to logger.info and returns False instead.

What do you think? If you want, I could look into preparing a pull request for that.

Thinking this further, I'd like to propose that: * Each possible reason for the validation to fail raises a specific exception, each of them subclassed from a `EmailValidationError`. * A new public function `check_email()` does all the checks, letting the exceptions through to the caller so the caller can decide what to do with that information, like even displaying a detailled error message in the user interface. * The existing function `validate_email()` just calls `check_email()`, catches all `EmailValidationError` exceptions, logs them to `logger.info` and returns `False` instead. What do you think? If you want, I could look into preparing a pull request for that.

👍 1

karolyi commented

2020-04-08 12:47:37 +02:00

(Migrated from github.com)

Sounds good to me, but I'd call the function something else that's more indicative of letting exceptions through.

Prepare a PR and I'll look into it. Also make sure you have tests for the new functionality.

Sounds good to me, but I'd call the function something else that's more indicative of letting exceptions through. Prepare a PR and I'll look into it. Also make sure you have tests for the new functionality.

reinhard-mueller commented

2020-04-09 00:01:30 +02:00

(Migrated from github.com)

See #8.