step line numbers are recorded when protocol is loaded (#65) (#121) #183
Conversation
This change depends on a fork of yaml-rust, https://github.com/hallettj/yaml-rust/tree/load_from_str_with_markers
soenkehahn
left a comment
soenkehahn
left a comment
There was a problem hiding this comment.
I think this PR should add tests for the feature that it's implementing, even though the feature is tested through other tests. So something like steps_fail_with_yaml_source_locations. Maybe even two tests that demonstrate that the source locations actually point to different places in the yaml file when two different steps fail.
src/protocol/marker.rs
Outdated
| @@ -0,0 +1,32 @@ | |||
| use yaml_rust; | |||
|
|
|||
| #[derive(Clone, Copy, PartialEq, PartialOrd, Debug, Eq, Ord, Hash)] | |||
There was a problem hiding this comment.
Do we need all these derived traits, or is this just your default list of things that you derive for everything?
There was a problem hiding this comment.
Yes, some of these I just copied over from the yaml-rust struct where they are necessary because markers form parts of hash keys. I'll push a change.
| pub struct Marker { | ||
| pub line: usize, | ||
| pub col: usize, | ||
| } |
There was a problem hiding this comment.
Why are we implementing our own Marker and don't reuse the one from yaml-rust?
There was a problem hiding this comment.
I construct Marker values in some tests. The fields in yaml_rust::Marker are private, and so is the constructor.
There was a problem hiding this comment.
Hmm, but shouldn't your changes to yaml-rust make those things public? If the first and only usage of the new yaml-rust version needs this to be public?
src/protocol/marker.rs
Outdated
| pub fn line(&self) -> usize { | ||
| let Marker { line, .. } = self; | ||
| *line | ||
| } |
There was a problem hiding this comment.
Why do we need a getter function for this? Couldn't we just use the field?
There was a problem hiding this comment.
Good point; I have removed the getter.
src/protocol/mod.rs
Outdated
| pub fn marker(&self) -> Option<Marker> { | ||
| let Step { marker, .. } = self; | ||
| *marker | ||
| } |
There was a problem hiding this comment.
Why not use the field marker?
There was a problem hiding this comment.
I have also removed this getter.
| #[derive(PartialEq, Eq, Debug, Clone)] | ||
| pub struct Step { | ||
| pub command_matcher: CommandMatcher, | ||
| pub marker: Option<Marker>, |
There was a problem hiding this comment.
What are the cases where this can be None?
There was a problem hiding this comment.
Markers come wrapped in Option from yaml-rust. We would have to use unwrap if we don't want the Option wrapper in Step.
In an early version of my changes to yaml-rust I did not have an option wrapper for markers. I remember having a good reason for adding the wrapper - something other than just the convenience of being able to implement From<Yaml> for Node. But I don't remember exactly what that reason was.
tests/run.rs
Outdated
| "error in {}.protocols.yaml: \ | ||
| expected: array, got: Integer(42)", | ||
| expected: array, \ | ||
| got: Node(Integer(42), Some(Marker {{ index: 10, line: 1, col: 10 }}))", |
There was a problem hiding this comment.
I don't think this test should be changed in this PR.
There was a problem hiding this comment.
Ok, I changed this back. But I think it would be useful to include the source location in this error message in a future change.
|
I added tests specific to line number reporting, |
|
@soenkehahn May I get a re-review? |
This change depends on a fork of yaml-rust,
https://github.com/hallettj/yaml-rust/tree/load_from_str_with_markers
@hallettj: Here's the PR with the cherry-picked commit.