Catch parameter transform errors #766
Conversation
aslakhellesoy
changed the base branch from
master
to
cucumber-expressions-2.0.0
There was a problem hiding this comment.
Awesome @aslakhellesoy! What do your think about merging this into #764 and adding #753 to that PR as well? They are all interrelated and dependent on one another anyway.
src/runtime/step_runner.js
Outdated
| error = stepDefinition.getInvalidCodeLengthMessage(parameters) | ||
|
|
||
| try { | ||
| const parameters = stepDefinition.getInvocationParameters({scenarioResult, step, parameterRegistry}) |
There was a problem hiding this comment.
I'd prefer we only have the try/catch around this statement. I like try/catch to be very isolated to prevent it from catching errors we don't want it to.
There was a problem hiding this comment.
Fixed in a071aab
Personally I'd merge them to master independently. They're related, but independent discrete improvements. I'd merge #764 first, then #766 (this one) and finally #753 (which would have to merged/rebased with master first). I'm going away for a week, so I won't have time to address the code review feedback on those PRs, but I agree with all of them, so feel free to fix them while I'm gone :-) |
aslakhellesoy
force-pushed
the
catch-parameter-transform-errors
branch
from
4f0d65f to
e5f317e
Compare
|
I found some time to address feedback. This should be ready to merge now I think. |
src/runtime/step_runner.js
Outdated
| error = err | ||
| } | ||
|
|
||
| if(!error) { |
There was a problem hiding this comment.
npm run eslint is happy. Feel free to add more strict rules if you think it's helpful.
There was a problem hiding this comment.
I do wish eslint caught this but regardless I would still like a space added here.
There was a problem hiding this comment.
Do you think it would be fair to call that out as pedantic?
There was a problem hiding this comment.
Yes. But I think it helps with readability which is important.
There was a problem hiding this comment.
Readability is important. But even more important is to display appreciation for contributions. When you nitpick like this it can easily be interpreted as "I'm never going to live up to this guy's standards", and that turns contributors away.
There was a problem hiding this comment.
We've all got our pet peeves, I certainly have mine. That doesn't mean we need to share them with everyone. If minor formatting issues like this bugs you I suggest you tweak the linting rules, or write your own if you can't find rules that are strict enough for your liking.
Or just cut people some white space slack and fix it yourself after you merge.
There was a problem hiding this comment.
I agree a linter would be ideal here but I don't really see the different between a linter and me asking for a whitespace change.
But even more important is to display appreciation for contributions.
Does me saying "Awesome @aslakhellesoy!" earlier in this PR not count? Anytime I merge a PR, I always have a "Thank you" comment after and try to comment on the PR letting the user know when the change has been released.
We've all got our pet peeves, I certainly have mine.
One of mine is to actually have all the keys of an object sorted. This was something that got messed up in a previous PR. That I did not think was worth asking you to do.
If minor formatting issues like this bugs you I suggest you tweak the linting rules, or write your own if you can't find rules that are strict enough for your liking.
I have done this in the past for other issues and if this becomes a common problem I would certainly look into it. But I haven't really run into this much as the general styling is all I'm asking. A line of whitespace between functions. A space between an if and the paren.
Thank you for the PRs. I'll take care of merging them and dealing the my comments
aslakhellesoy
force-pushed
the
cucumber-expressions-2.0.0
branch
from
2df2293 to
72c3d4a
Compare
aslakhellesoy
force-pushed
the
catch-parameter-transform-errors
branch
from
a071aab to
26169b5
Compare
|
I've rebased this on top of #764 - should be ready to merge |
src/runtime/step_runner.js
Outdated
| error = err | ||
| } | ||
|
|
||
| if(!error) { |
There was a problem hiding this comment.
I do wish eslint caught this but regardless I would still like a space added here.
charlierudolph
changed the base branch from
cucumber-expressions-2.0.0
to
master
# Conflicts: # features/custom_parameter.feature # src/runtime/step_runner.js
|
This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue for related bugs. |
This is a fix for #765. It depends on #764 and is branched off from the
cucumber-expressions-2.0.0branch.