-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Check required-version before parsing rules #22410
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
Conversation
|
Nice |
|
|
Hmm, the ecosystem changes are a bit surprising. Can you try rebasing on main? |
|
Hm do those changes mean it found more autofixes? That would be weird. I'll apply your suggested change and merge in with latest main. |
MichaReiser
left a comment
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. This is great and nice find with DeTable.
I only have a question whether we need the required_version fallback and if we retain the spans in error messages if the required version fails to parse.
| let required_version = RequiredVersion::try_from(required_version.to_string()) | ||
| .with_context(|| format!("Failed to parse {}", path.display()))?; |
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.
I suspect that we lose the span in the error message compared to when deserializing the value as part of the serde deserialization. I'm inclined to return Ok here and defer to the "full" deserialization in that case.
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.
I'll add a test for this case too.
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.
You were right, changed it as you suggested.
|
Thanks for the review and merge @MichaReiser! |
Summary
Fixes #19922.
The approach I used is to check the required-version separately before parsing the TOML. This feels a bit ugly because we have to parse the TOML separately, but other approaches I considered seemed worse:
Test Plan
New test cases added.