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

Poor error message/type for validators.in_ with a string #382

Closed
Zac-HD opened this issue May 12, 2018 · 7 comments
Closed

Poor error message/type for validators.in_ with a string #382

Zac-HD opened this issue May 12, 2018 · 7 comments

Comments

@Zac-HD
Copy link
Contributor

Zac-HD commented May 12, 2018

import attr

@attr.s
class C:
   s = attr.ib(validator=attr.validators.in_('abc'))

C(s=1)  # TypeError: 'in <string>' requires string as left operand, not int

__contains__ behaves a little weirdly for strings, but I still think this error could be improved.

Obvious options would be to catch exceptions and re-raise something with a clearer error message, and/or to deprecate use of strings as the collection here.

Found in attrs=18.1.0 while working on HypothesisWorks/hypothesis#954.

@hynek
Copy link
Member

hynek commented May 14, 2018

What is the actual problem here? I don't think it's fair to expect in_ to also type check?

I think you want

s = attr.ib(validator=[attr.validators.instance_of(str), attr.validators.in_('abc')])

?

@wsanchez
Copy link

I think the request is for something like:

    def __call__(self, inst, attr, value):
        if value not in self.options:
            try:
                raise ValueError(
                    "'{name}' must be in {options!r} (got {value!r})"
                    .format(name=attr.name, options=self.options, value=value)
                )
            except Exception as e:
                raise AssertionError(
                    "in_ validator for attribute {attr} raised exception for value {value}: {error}"
                    .format(attr=attr, value=value, error=e)
                )

@Zac-HD
Copy link
Contributor Author

Zac-HD commented May 15, 2018

Almost - the idea is that if value in self.options raises an exception instead of returning a bool, we should still raise a ValueError (not e.g. TypeError), and preferably one with a useful and consistent message.

I'd write this as follows, and would be happy to open a PR if that would be helpful 😄

def __call__(self, inst, attr, value):
	try:
		if value not in self.options:
			raise ValueError(
				"'{name}' must be in {options!r} (got {value!r})"
				.format(name=attr.name, options=self.options, value=value)
			)
	except Exception:
		raise ValueError(
			"'{name}' must be in {options!r} "
			"(got {value!r}, which caused an internal error)"
			.format(name=attr.name, options=self.options, value=value)
		)

@wsanchez
Copy link

The contract for validators isn't super clear in the docs, but I don't see anything that says that ValueError is the only acceptable sort of exception to raise. That probably should be clarified, but I would think TypeError is a valid exception to raise in the case where data of the wrong type is given, as in this example.

I do agree that the a better error message would be useful, but I'd definitely include the original exception text.

@hynek
Copy link
Member

hynek commented May 16, 2018

Uh yeah there is absolutely no contract about what kind of exceptions are raised by validators and I’d even argue that changing it could be backward incompatible. 🤔

@wsanchez
Copy link

I think it might make some sense for us to advise that certain exceptions are used, but I wouldn't go farther than that. Otherwise, every validator would have to wrap a try/except around it's body, which could be a best practice, but seems like a lame thing to require.

Which leaves us with: should we do better than the above error message. I notice I wrapped the try/except in my example around the wrong code (oops).

Lemme try again with a different suggestion:

    def __call__(self, inst, attr, value):
        try:
            in_options = value in self.options
        except TypeError as e:
            in_options = False

        if not in_options:
            raise ValueError(
                "'{name}' must be in {options!r} (got {value!r})"
                .format(name=attr.name, options=self.options, value=value)
            )

I don't think trying to catch all exceptions is necessarily more correct, but we can definitely say that a non-string object isn't in the provide bucket (that happens to be a string) of options. Since that bucket isn't required to contain objects of a homogenous type, I returning False is more appropriate (or consistent with, say a list) than raising a TypeError.

@hynek
Copy link
Member

hynek commented May 23, 2018

I guess I would accept a PR in this case.

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

No branches or pull requests

3 participants