Better Yaml Errors #499
Better Yaml Errors #499
Conversation
|
I need to do a rebase on this, but it's in a state to be opened for thoughts either way |
rydrman
force-pushed
the
better-yaml-errors
branch
from
73f5dca to
9c58108
Compare
|
Okay, this is up-to-date now |
Codecov Report
@@ Coverage Diff @@
## clippy-updates #499 +/- ##
==================================================
+ Coverage 67.54% 67.70% +0.16%
==================================================
Files 276 278 +2
Lines 18869 19148 +279
==================================================
+ Hits 12745 12965 +220
- Misses 6124 6183 +59
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| // unrecognized fields are explicitly ignored in case | ||
| // they were added in a newer version of spk. We assume | ||
| // that if the api has not been versioned then the desire | ||
| // is to continue working in this older version |
There was a problem hiding this comment.
Is it possible to know the spec version at this point? Someone could construct a spec file that has the version specified at the bottom of the file. I'm just thinking that if the spec version is known then this could be pickier about seeing unexpected content.
I would like a warning emitted for unexpected content to help catch typo bugs too. But it would be nice if those were enabled only in some contexts, like parsing a spec during a build, so these kinds of mistakes are caught early during dev but don't pester the end users of broken packages that they don't control.
There was a problem hiding this comment.
ya, not without requiring that the spec version be at the top of the file (which we could do, but is not really to the yaml spec). Actually, even in that case, we would not have a good way to pass that information into this deserialize code - not impossible but I imagine it would be quite involved...
| _ => { | ||
| // for forwards compatibility we ignore any unrecognized | ||
| // field, but consume it just the same | ||
| // TODO: could we check for possible typos in here? |
There was a problem hiding this comment.
Continuing my thoughts above, if these are warnings that get logged to a specific different channel then that channel can be enabled during a build so devs see them, and then not emitted when parsing packages during normal solves.
There was a problem hiding this comment.
I agree - there are a lot of places that we could use a separate tracing level/target for build-time warnings to devs. I haven't looked into how one might do that with the tracing package, but even just a labelled span for parsing-from-a-file plus some extra logic at log initialization might do...
Signed-off-by: Ryan Bottriell <[email protected]>
Signed-off-by: Ryan Bottriell <[email protected]>
Signed-off-by: Ryan Bottriell <[email protected]>
Signed-off-by: Ryan Bottriell <[email protected]>
Signed-off-by: Ryan Bottriell <[email protected]>
Using the serde_yaml crate directly is not advisable anymore because the errors need to be wrapped in order to use the extended format_serde_error functionality. A simply FromYaml trait with a blanket implementation takes care of this for us while also supporting the custom double-parsing that's required by the api versioning Signed-off-by: Ryan Bottriell <[email protected]>
Signed-off-by: Ryan Bottriell <[email protected]>
Signed-off-by: Ryan Bottriell <[email protected]>
…g types Signed-off-by: Ryan Bottriell <[email protected]>
Signed-off-by: Ryan Bottriell <[email protected]>
Signed-off-by: Ryan Bottriell <[email protected]>
Signed-off-by: Ryan Bottriell <[email protected]>
Signed-off-by: Ryan Bottriell <[email protected]> Co-authored-by: jrray <[email protected]>
rydrman
force-pushed
the
better-yaml-errors
branch
from
fc8f57e to
bfb987a
Compare
|
I'll merge this once #514 goes in |
The goal was to create error messages which were longer and more expressive during the yaml deserialization process. I'm using an existing crate which nicely formats the yaml errors with positional information and original context (with color).
I think there's probably much more that we could do, but this first pass required replacing all of the previous custom deserialization code with proper
serde::de::Visitortypes that allow the deserializer to retain positional information for errors. I've also done my best to validate data as much as possible during the first pass of the deserializer so that any issues are flagged in line to the actual issue in yaml - though this is not possible across the board because of some of the validation that we do.We also still have the issue of unhelpful errors from nom, but this should help place them in the yaml to help with context.