Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve the error message informing the value that it got it #536

Conversation

williamjamir
Copy link
Contributor

I would like to propose a change on the error message returned by the validator is_callable().

Currently, only a message "TypeError: 'x' must be callable" it's informed to the user, I would like to add the value that was passed to it within the message.

This approach besides given more information to the user resembles the errors messages returned by others validators that also include the value informed =)

Pull Request Check List

This is just a reminder about the most common mistakes. Please make sure that you tick all appropriate boxes. But please read our contribution guide at least once, it will save you unnecessary review cycles!

If an item doesn't apply to your pull request, check it anyway to make it apparent that there's nothing to do.

  • Added tests for changed code.
  • New features have been added to our Hypothesis testing strategy.
  • Changes or additions to public APIs are reflected in our type stubs (files ending in .pyi).
    • ...and used in the stub test file tests/typing_example.py.
  • Updated documentation for changed code.
    • New functions/classes have to be added to docs/api.rst by hand.
    • Changes to the signature of @attr.s() have to be added by hand too.
    • Changed/added classes/methods/functions have appropriate versionadded, versionchanged, or deprecated directives.
  • Documentation in .rst files is written using semantic newlines.
  • Changes (and possible deprecations) have news fragments in changelog.d.

If you have any questions to any of the points above, just submit and ask! This checklist is here to help you, not to deter you from contributing!

@williamjamir williamjamir force-pushed the improve-error-message-iscallable-validator branch 2 times, most recently from 58ddbfc to 7efdc3c Compare May 30, 2019 16:44
@hynek
Copy link
Member

hynek commented Jun 2, 2019

Hm I wish it would work like that but I'm afraid that this is a breaking change. :( Maybe make it an option?

@williamjamir
Copy link
Contributor Author

Hi hynek, thanks for your time review it :)

Hm I wish it would work like that but I'm afraid that this is a breaking change. :(

Oh sorry, I didn't consider that users would be relying/depending on the errors message raised by the validators.

Maybe make it an option?

Sure, do you have anything in mind? I mean, where would I introduce this option?

@wsanchez
Copy link

wsanchez commented Jun 4, 2019

If we want people to distinguish this TypeError from others without looking at string values, and the value of the thing-that-isn't-callable is useful, then that should all be made available without having to parse a string.

So perhaps a new exception type NotCallableError, subclassing TypeError, which has a message attribute, a value attribute, and make sure that it's __str__ produces the same not-informative-enough message as before?

@hynek
Copy link
Member

hynek commented Jun 5, 2019

What @wsanchez said SGTM!

@williamjamir
Copy link
Contributor Author

williamjamir commented Jun 8, 2019

Hi @wsanchez, I not sure if I got all right.

If we want people to distinguish this TypeError from others without looking at string values, and the value of the thing-that-isn't-callable is useful, then that should all be made available without having to parse a string.

Yep, I totally agree =)

So perhaps a new exception type NotCallableError, subclassing TypeError, which has a message attribute, a value attribute, and make sure that it's __str__ produces the same not-informative-enough message as before?

Right, I agree with adding this new exception (and I can add in the next commit without any problem), but the "original issue" that I brought was not solved, the message still not-informative to the "end user" that informed an invalid value.

So just to make sure that I got right, we are going to keep with the not-informative-enough message in order to avoid breaking changes right?

@wsanchez
Copy link

Yeah, I think @hynek is saying that the current string can't change for compatibility, so adding a new type that at least can get you the data you need is an improvement.

I agree that the default string could be better, and I'm less sympathetic to compatibility with code that's inspecting the string, but attrs has a high compatibility bar. (I'm also not sure where an option to enable an improved message would go.)

@hynek
Copy link
Member

hynek commented Jun 11, 2019

Firstly, having a dedicated class with a got field (do have have precedents on how to call fields like that?) allows you to write tests that don't break on changes. I guess I can live with changing the string in the end however if we do it, we should also add a good way to introspect the error.

@williamjamir williamjamir force-pushed the improve-error-message-iscallable-validator branch 2 times, most recently from b0f22f5 to b1961f9 Compare June 17, 2019 13:57
@williamjamir williamjamir force-pushed the improve-error-message-iscallable-validator branch 2 times, most recently from 8c815c7 to 3bfb793 Compare June 17, 2019 17:17
tests/test_exceptions.py Outdated Show resolved Hide resolved
src/attr/exceptions.pyi Outdated Show resolved Hide resolved
tests/test_validators.py Show resolved Hide resolved
tests/test_validators.py Show resolved Hide resolved
tests/test_validators.py Show resolved Hide resolved
changelog.d/536.change.rst Outdated Show resolved Hide resolved
@williamjamir williamjamir force-pushed the improve-error-message-iscallable-validator branch from 4ddf28b to bd7e3bb Compare June 23, 2019 14:05
@williamjamir williamjamir force-pushed the improve-error-message-iscallable-validator branch from bd7e3bb to 8f039da Compare June 23, 2019 14:24
@williamjamir
Copy link
Contributor Author

williamjamir commented Jul 4, 2019

Is there any further modification needed to proceed with the merge?

@hynek hynek merged commit 68af33e into python-attrs:master Jul 20, 2019
@hynek
Copy link
Member

hynek commented Jul 20, 2019

All good, thank you for your patience and your contribution!

@williamjamir
Copy link
Contributor Author

Thank you, I'm glad that I could I contribute with this project somehow.=)

@hynek
Copy link
Member

hynek commented Jul 20, 2019

I hope my lagginess doesn't discourage you from doing it more often. This year was awesome, but super stressful. 🙈

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants