Propagate Step being matched by multiple Regexes as AmbiguousMatchError #143
Conversation
FCM |
# Conflicts: # src/cucumber.rs
# Conflicts: # CHANGELOG.md # Cargo.toml # src/cli.rs # src/cucumber.rs # src/runner/basic.rs
# Conflicts: # CHANGELOG.md # src/event.rs # src/runner/basic.rs # src/writer/basic.rs
# Conflicts: # src/step.rs
ilslv
changed the title
| Some(::cucumber::step::Location { | ||
| path: ::std::convert::From::from(::std::file!()), | ||
| line: ::std::line!(), | ||
| column: ::std::column!(), |
There was a problem hiding this comment.
Given that we always provide cucumber::step::Location in the generated code, I think we may strip Option out of many places in of codegen machinery.
src/event.rs
Outdated
| /// | ||
| /// [`Regex`]: regex::Regex | ||
| /// [`Step`]: gherkin::Step | ||
| AmbiguousMatch(step::AmbiguousMatchError), |
There was a problem hiding this comment.
@ilslv can we just reuse Failed variant instead of introducing a new one over the whole code base, and try to downcast into step::AmbiguousMatchError in writer::Basic to provide a better printing?
There was a problem hiding this comment.
@tyranron we can, but I've would like to have this error encoded inside event. Mostly to show that this invariant is checked on a type level and not expect our users to read all docs or book. We can use something like this inside Failed variant
enum StepError {
AmbiguousMatch(..),
Panic(Arc<Info>),
}There was a problem hiding this comment.
@ilslv yes, this is better than introducing a whole new event variant.
src/step.rs
Outdated
| then: HashMap::new(), | ||
| given: BTreeMap::new(), | ||
| when: BTreeMap::new(), | ||
| then: BTreeMap::new(), |
There was a problem hiding this comment.
It's unclear why we've switched to BTreeMap instead of HashMap.
There was a problem hiding this comment.
@tyranron output test now requires stable ordering of Steps to show, which of them were matched inside AmbiguousMatchError. HashMap couldn't provide it, because of path inside of a Location and how rust tests work with names like somerandomjbrsh-output.
There was a problem hiding this comment.
@ilslv could this be handled by tests somehow, not on the library level?
There was a problem hiding this comment.
@tyranron yes, we can rearrange AmbiguousMatchError in unpacked event and only then output it.
There was a problem hiding this comment.
@ilslv then let's hack it in tests 👍
src/step.rs
Outdated
| } else { | ||
| // Instead of `.unwrap()` to avoid documenting `# Panics` | ||
| // section. | ||
| unreachable!() |
There was a problem hiding this comment.
.unwrap_or_else(|| unreachable!())? 🌚
src/runner/basic.rs
Outdated
| self.steps = mem::take(&mut self.steps).given(regex, step); | ||
| pub fn given( | ||
| mut self, | ||
| loc: Option<step::Location>, |
There was a problem hiding this comment.
I think having this in runner::Basic and Cucumber methods has no sense, because user will unlikely specify Locations by hand, while the proc-macro machinery does this via .steps() method.
Synopsis
For now if
Stepis matched by multipleRegexes, we just use one of them, which can lead to very non-intuitive and frustrating errors.Solution
Propagate that case as an
AmbiguousMatchError.Checklist
Draft:prefixDraft:prefix is removedThis PR is nominated at RU RustCon Contest