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 error message for ValidationError #218

Merged

Conversation

lu-fennell
Copy link
Contributor

@lu-fennell lu-fennell commented Oct 30, 2021

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.

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 Reviewable

@lu-fennell lu-fennell force-pushed the improved-validation-error-messages-simple-pr branch from 5b6df93 to b1c5978 Compare November 1, 2021 00:58
Copy link
Owner

@jonhoo jonhoo left a 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.

src/error.rs Outdated Show resolved Hide resolved
src/client.rs Outdated Show resolved Hide resolved
@lu-fennell lu-fennell force-pushed the improved-validation-error-messages-simple-pr branch from b1c5978 to 53ecab3 Compare November 7, 2021 16:45
Copy link
Owner

@jonhoo jonhoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent, thank you!

@jonhoo
Copy link
Owner

jonhoo commented Nov 10, 2021

Now we just need CI to cooperate. I've just pushed a commit to master that should fix some of it. The tarpaulin failure is due to xd009642/tarpaulin#756, which we're still bit by only because we need to update our dependencies. Bumping the currently-outdated dependencies in Cargo.toml should take care of it (though let's do that in a separate PR):

$ 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 rustfmt on beta. That's optional to fix, though good to do ahead of time now that we know about it.

@jonhoo
Copy link
Owner

jonhoo commented Nov 10, 2021

(though please do rebase onto or merge with master just to make sure we didn't miss any regressions)

@lu-fennell lu-fennell force-pushed the improved-validation-error-messages-simple-pr branch from 53ecab3 to 1c53c0a Compare November 10, 2021 08:05
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.
@lu-fennell lu-fennell force-pushed the improved-validation-error-messages-simple-pr branch from 1c53c0a to 8147f17 Compare November 10, 2021 09:12
@lu-fennell
Copy link
Contributor Author

Ok, done: rebased and fixed formatting.

Copy link
Owner

@jonhoo jonhoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@jonhoo jonhoo merged commit 6808dfe into jonhoo:master Nov 11, 2021
@lu-fennell lu-fennell deleted the improved-validation-error-messages-simple-pr branch November 12, 2021 15:42
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.

Better error messages for validation errors
2 participants