Enhance ValueError messages (#148 and #157)

[LaurentAjdnik]

For feedback: First attempt to enhance ValueError messages (see #148 and #157), starting with register.py.

Guidelines:

• Functional explanation of parameter
• Technical name of parameter
• Provided value of parameter
• Using "must" rather than any other verb
• No negation
• Using "less than / equal to / greater than" rather than any other expression
• Recall allowed values
• All sentences follow the same structure

[HGSilveri]

No complaints, I agree that this is much cleaner

[HGSilveri]

Cool, I didn't know about this syntax

[HGSilveri]

@LaurentAjdnik So, how do you want to go about this? Doing this for the whole code seems like a big task. But also, I was wondering about how we can enforce it automatically, for anyone contributing to the code base.

I had an idea to have a test in CI that would search the code for specific errors and check if their message is valid. For example, it would identify "threshold" value errors and check if the message follows the specific guidelines. What do you think about this?

[LaurentAjdnik]

@LaurentAjdnik So, how do you want to go about this? Doing this for the whole code seems like a big task.

Still, I had planned to do it. 😇

But also, I was wondering about how we can enforce it automatically, for anyone contributing to the code base.

Automatically? I have not clue... I can only think of writing guidelines in CONTRIBUTING.md and checking if it's done when a PR arrives...

I had an idea to have a test in CI that would search the code for specific errors and check if their message is valid. For example, it would identify "threshold" value errors and check if the message follows the specific guidelines. What do you think about this?

Cool! How would you do that?

[HGSilveri]

@LaurentAjdnik So, how do you want to go about this? Doing this for the whole code seems like a big task.

Still, I had planned to do it. 😇

Thanks a lot for the initiative!

I had an idea to have a test in CI that would search the code for specific errors and check if their message is valid. For example, it would identify "threshold" value errors and check if the message follows the specific guidelines. What do you think about this?

Cool! How would you do that?

First, we have to layout all the guidelines for the error messages. At first, we could try to check for specific patterns that all errors of a given type should follow, based on these guidelines. This will be easier said than done, considering the variability that can occur within a single error type (eg ValueError). Depending on how far we want to take this, it might be necessary to create custom subclasses to further specify the error involved (e.g. ValueThresholdError, etc...), though this might be overkill for now.

[HGSilveri]

@LaurentAjdnik do you have anything to add to this PR before I merge it?

[LaurentAjdnik]

@LaurentAjdnik do you have anything to add to this PR before I merge it?

Nope.

[HGSilveri]

LGTM

[LaurentAjdnik]

First, we have to layout all the guidelines for the error messages. At first, we could try to check for specific patterns that all errors of a given type should follow, based on these guidelines. This will be easier said than done, considering the variability that can occur within a single error type (eg ValueError). Depending on how far we want to take this, it might be necessary to create custom subclasses to further specify the error involved (e.g. ValueThresholdError, etc...), though this might be overkill for now.

Indeed, it does sound like a lot of hassle for something not so worth it.

I guess we should keep it simple for now: Let's officialize some guidelines, make sure all existing ValueErrors are updated accordingly (*) and check any PR coming in.

(*) Most of the time, when in doubt, people check existing parts of code to make sure they comply to guidelines. That's what I did anyway. 😉

[HGSilveri]

I guess we should keep it simple for now: Let's officialize some guidelines, make sure all existing ValueErrors are updated accordingly (*) and check any PR coming in.

(*) Most of the time, when in doubt, people check existing parts of code to make sure they comply to guidelines. That's what I did anyway. 😉

I agree. My only concern is the compliance with the guidelines, and the need for the reviewer to remember to check for them. In general, I prefer to have as much checks as possible happening automatically, so as not to pile on the things the reviewer as to look for. But I agree that, at least for now, what you suggest seems sufficient.