-
Notifications
You must be signed in to change notification settings - Fork 84
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 error message for ValidationError
#218
Improve error message for ValidationError
#218
Conversation
5b6df93
to
b1c5978
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a fantastic change and PR, thank you!
I left two comments inline that I think we should address, but otherwise this is good to land.
b1c5978
to
53ecab3
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Excellent, thank you!
Now we just need CI to cooperate. I've just pushed a commit to $ cargo outdated -d1
Name Project Compat Latest Kind Platform
---- ------- ------ ------ ---- --------
imap-proto 0.14.3 --- 0.15.0 Normal ---
nom 6.2.1 --- 7.1.0 Normal ---
ouroboros 0.9.5 --- 0.13.0 Normal ---
regex 1.4.6 --- 1.5.4 Normal ---
rustls-connector 0.13.1 --- 0.15.0 Normal --- The only change that actually seems related to this PR is the failure of |
(though please do rebase onto or merge with master just to make sure we didn't miss any regressions) |
53ecab3
to
1c53c0a
Compare
In addition to the invalid character, the error now also contains a `command_synopsis` and `argument` which identifies the command and argument that failed to validate.
1c53c0a
to
8147f17
Compare
Ok, done: rebased and fixed formatting. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
In addition to the invalid character, the error now also contains a
command_synopsis
andargument
which identifies the command andargument that failed to validate.
Fixes #215
Currently the error message just shows a "synopsis" of the command, e.g.,
FETCH seq query
, not the actual arguments. Maybe it makes sense to also show the invalid argument value.. except if it is a secret of course. If you think this is a good idea, and provided this PR goes into the right direction, I can add this functionality also.This change is