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

Reference values in error messages. Fixes #4. #17

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Rogdham
Copy link
Contributor

@Rogdham Rogdham commented May 13, 2013

No description provided.

@Rogdham
Copy link
Contributor Author

Rogdham commented May 13, 2013

@halst: Travis build failed because of the detected test in the readme…

Schema(set([int, float])).validate(set([5, 7, 8, 'not int or float here']))

was supposed to fail like

SchemaError: Or(<type 'float'>, <type 'int'>) did not validate 'not int or float here'
'not int or float here' should be instance of <type 'float'>

but instead fails like

SchemaError: Or(<type 'float'>, <type 'int'>) did not validate 'not int or float here'
'not int or float here' should be instance of <type 'int'>

and it does not matter, and since this comes from a set, I don't know if we can assume something about the order.

Could you look at it? I believe this problem was already in current master. I think that the best way to “fix” it is to make Travis ignore the implicit tests from the readme file.

If you fix it, ping me if you want me to rebase my PR. Same problem on #18.

@keleshev
Copy link
Owner

Yes, it's ok that Travis is failing. However, I need to think about this feature.

@keleshev
Copy link
Owner

I'm just thinking, what if someone wants to use format method on the error message. It would look awkward:

Use(int, error='Invalid {}: {{value}}'.format('year'))

@Rogdham
Copy link
Contributor Author

Rogdham commented May 14, 2013

For the removal of error=e, there are different reasons for each of them (lines numbers are the one after my commit):

line 115: my bad, I shouldn't have remove that one; I'm changing my commit

line 125: completely useless, since the exception is caught line 132, and is forgotten afterwards; I've added a comment to reflect that

line 127: this one is more difficult to analyse. The exception is caught but re-raised, and the errors get added on line 141.
If we don't provide an error message, it's ok and normal (behaviour not changed by this PR):

>>> Schema({int:str}).validate({4:2})
Traceback (most recent call last):
…
schema.SchemaError: key 4 is required
2 should be instance of <type 'str'>

If we provide an error message without the {value} (e.g. previous behaviour before this PR), everything went fine as well (and I'm explaining why in the next case) (behaviour not changed by this PR):

>>> Schema({int:str}, error='Error message').validate({4:2})
Traceback (most recent call last):
…
schema.SchemaError: Error message

But if we provide an error message with a {value}, it's not as we probably want it to be (behaviour as before this PR):

>>> Schema({int:str}, error='Error value: {value}').validate({4:2})
Traceback (most recent call last):
…
schema.SchemaError: Error value: {4: 2}
Error value: 2

As you can see, it's getting reported twice. For real errors messages, it's probably not what we want, as it requires modification afterwards. The fact is that in the previous case (without {value}), it was reported twice too, but we had a duplicate removal on line 27 witch took care about that.

Note that it is always possible to have both reports, with {value}, if we specify the internal error message (behaviour not changed by this PR):

>>> Schema({int:Use(int, error='Internal error: {value}')}, error='External error: {value}').validate({4:'a'})
Traceback (most recent call last):
…
schema.SchemaError: External error: {4: 'a'}
Internal error: a

So what this removal of error=e effectively does is avoiding to report external error messages when we got an internal error message (the external error message getting used for the external error in that case though, but this is what we want).

@Rogdham
Copy link
Contributor Author

Rogdham commented May 14, 2013

I'm just thinking, what if someone wants to use format method on the error message. It would look awkward:
Use(int, error='Invalid {}: {{value}}'.format('year'))

Absolutely. I don't know how to improve that however. Not sure that it would be done a lot though.

In some cases, it could be better to catch the exception, since the .value is provided:

>>> try:
...     Schema(int).validate('foo')
... except SchemaError as e:
...     print '{}: {}'.format('year', e.value)
... 
year: foo

An other idea could be to detect if the error argument is callable, and in that case call it with the value…
Would allow things like: Use(int, error=lambda value: 'Invalid {}: {}'.format('year', value))
But it sounds like overkill.

@skorokithakis
Copy link
Collaborator

Could you take a look at #107 as well? I think it relates closely to this PR.

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