Upgrade to Cucumber Expressions 3.0.0 #764

aslakhellesoy · 2017-02-17T14:11:25Z

This PR upgrades to Cucumber Expressions 3.0.0, which has several improvements.

This PR also removes the custom logic to compute parameter names in snippet functions, and uses the logic provided by the cucumber-expressions library instead.

jbpros

This looks great!

The build is failing because of ES features (class) used by cucumber-expressions that are not available on Node 4.

Should we just add cucumber-expressions to the files transpiled by babel?

jbpros · 2017-02-17T14:18:10Z

.gitignore

@@ -3,3 +3,4 @@
 lib/
 node_modules
 usage.txt
+.idea/


I'd prefer if we avoided IDE-specific exclusions.

It's often recommended to do that locally so that we don't have to worry about everyone's preferred IDEs/editors.

If we add it to the repo, then we don't have to worry about some git noob adding their whole IDE config folder in a PR.

Is this really worth worrying about?

If a "git noob" sends a PR with the whole .idea folder checked in, it would be a great opportunity to teach them a bit more about Git 👍

But no, it's probably not worth discussing any further. I told you my preference, you decide.

I'm with @jbpros here. I believe anything that is not universal to this project belongs in your local global gitignore.

Just my two cents: We don't have to "worry" about everyone's preferred IDE, if we just accept their addition to the gitignore whenever it comes. Also, I'm guessing this won't be the last time someone tries to add .idea/ to the gitignore.

But I agree, either way this isn't worth worrying about lol

charlierudolph · 2017-02-17T15:48:29Z

src/support_code_library/builder.js

+      console.error('addTransform is obsolete and will be removed in a future version. Please use addParameter instead.')
+    }
+    addParameter({captureGroupRegexps, transformer, typeName})
+  }


I'd prefer not to have function definitions in here. Can these be moved into helpers? Similar to how After/Before/defineStep work.

Done in 47d54ec

charlierudolph · 2017-02-17T15:48:49Z

src/support_code_library/builder.js

+    // eslint-disable-next-line no-console
+    if (console !== 'undefined' && typeof console.error === 'function') {
+      // eslint-disable-next-line no-console
+      console.error('addTransform is obsolete and will be removed in a future version. Please use addParameter instead.')


Please use util.deprecate here

Oh nice, wasn't aware of that one.

Done in d4d4728

charlierudolph · 2017-02-17T15:50:38Z

features/custom_parameter.feature

+      })
+      """
+    When I run cucumber.js
+    Then the step "a passing step" has status "passed"


Please add a newline to the end of this file. Thanks for writing this feature test!

Done in d7642f0

aslakhellesoy · 2017-02-17T16:00:55Z

The cucumber-expressions library's main script is dist/index.js, which is transpiled by Babel and included in the npm package. Looks like it's not ES5 code though. I'm going to have to tweak cucumber-expressions' Babel settings.

charlierudolph · 2017-02-19T18:06:11Z

@aslakhellesoy I'm uncertain about changing the API to addParameter. That method name doesn't tell me enough about what it does. Thats the internal cucumber expression name but doesn't need to be the public facing API. I envision this being used mostly for actual transforms instead of merely naming certain capture group regular expressions but I could be wrong. I believe the transformer function is required at the moment too.

aslakhellesoy · 2017-02-26T20:49:15Z

The {...} parts of cucumber expressions are called parameters (the equivalent of a RegExp capture group).

Custom parameter types can be registered, and they have a name, pattern (or list of patterns) and a transform function. For convenience there are a couple of "built-in" transforms for float and int.

It's important to establish a ubiquitous language for the various Cucumber implementations, both for the API and the documentation. Ideally Cucumber would provide an API that is identical to the "internal" Cucumber Expressions API. I did design the external API of Cucumber Expressions to be "user friendly". I think parameter and parameter type are better names than transform for a few reasons.

The term parameter is a familiar concept in programming. It allows us to say things like: "The I have {int} cukes in my {bodypart} expressions has two parameters". It would be weird to say it has two transforms.

A parameter doesn't necessarily have to transform a string - it could leave it unchanged. A custom parameter is still useful in many cases, typically when we want to restrict matching to a particular pattern, such as /red|blue|yellow/. (Currently the API requires an explicit transform function, but we could make that optional (and default an identity function) in a future release of Cucumber Expressions.

Maybe it would be better to rename addParameter to defineParameterType, because we're not really adding a parameter, we're defining a parameter type that can be used by parameters in expressions.

Regarding the Cucumber API - I think it would be better to use positional arguments (as described in the docs) rather than named ones. The addParameter method is polymorphic - the 3rd argument, captureGroupRegexps can be a RegExp or an array of RegExp. In most cases you only need a single RegExp, and it looks weird to say captureGroupRegexps: /red|blue|yellow/. In general I do like named parameters in methods, but I think this is an exception.

aslakhellesoy · 2017-03-01T23:46:52Z

What are your thoughts on this @charlierudolph ?

charlierudolph · 2017-03-01T23:56:26Z

I like the rename to defineParameterType. In general I don't like positional arguments when you have 4 or more arguments. I think it makes the function call too hard to read. With regards to captureGroupRegexs. We could also support captureGroupRegex and stipulate that you must specify one and only one.

aslakhellesoy · 2017-03-02T00:31:38Z

Ok, sounds good. I hope to release a new Cucumber Expressions in a couple of days with those renames, and a few other goodies. I'll update this PR after that, and then we should be good to go! (Plus the two other PRs I've submitted that depend on this one).

…arameterType

… between quotes.

aslakhellesoy · 2017-03-09T21:25:58Z

I've done the renames, can you take another look?

charlierudolph

Looks great. Thanks @aslakhellesoy

charlierudolph · 2017-03-09T21:30:54Z

src/support_code_library/builder.js

 import * as helpers from './helpers'
-


please add this newline back

charlierudolph · 2017-03-09T21:34:16Z

.gitignore

@@ -3,3 +3,4 @@
 lib/
 node_modules
 usage.txt
+.idea/


Can you please remove this? I strongly agree with @jbpros and would prefer to take that opportunity to teach people to use their local global gitignore.

charlierudolph · 2017-03-09T21:36:36Z

features/custom_parameter.feature

@@ -0,0 +1,69 @@
+Feature: Custom Parameter


Please use parameter type instead of parameter in this title, the feature description, and the scenario titles.

Actually looks like this was done in one of the following PRs so this can be ignored.

lock · 2018-10-25T05:16:43Z

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.

aslakhellesoy requested review from jbpros and charlierudolph February 17, 2017 14:11

jbpros reviewed Feb 17, 2017

View reviewed changes

charlierudolph reviewed Feb 17, 2017

View reviewed changes

charlierudolph mentioned this pull request Feb 17, 2017

Allow async transforms #753

Closed

aslakhellesoy mentioned this pull request Feb 18, 2017

Catch parameter transform errors #766

Closed

aslakhellesoy changed the title ~~Upgrade to Cucumber Expressions 2.0.0~~ Upgrade to Cucumber Expressions 2.0.1 Feb 26, 2017

aslakhellesoy added a commit to cucumber/common that referenced this pull request Mar 8, 2017

API renames, based on feedback in cucumber/cucumber-js#764

d364c4b

aslakhellesoy added 7 commits March 9, 2017 09:40

Upgrade to Cucumber Expressions 2.0.0

dbdca3b

Upgrade to cucumber-expressions 2.0.1 to fix Node 4 build

bfecc83

Use util.deprecate instead of homegrown deprecation

e53b416

Extract addParameter to helpers

ba63857

EOF newline

e6d5495

Upgrade to cucumber-expressions 3.0.0, rename addParameter to defineP…

afed98d

…arameterType

Update docs, make stringInDoubleQuotes require at least one character…

72c3d4a

… between quotes.

aslakhellesoy force-pushed the cucumber-expressions-2.0.0 branch from 2df2293 to 72c3d4a Compare March 9, 2017 09:59

aslakhellesoy changed the title ~~Upgrade to Cucumber Expressions 2.0.1~~ Upgrade to Cucumber Expressions 3.0.0 Mar 9, 2017

charlierudolph approved these changes Mar 9, 2017

View reviewed changes

update

6815c0f

charlierudolph closed this in 37a2014 Mar 9, 2017

charlierudolph deleted the cucumber-expressions-2.0.0 branch March 9, 2017 23:54

aslakhellesoy mentioned this pull request Mar 10, 2017

Non Capture Group Transform #784

Closed

lock bot locked as resolved and limited conversation to collaborators Oct 25, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Upgrade to Cucumber Expressions 3.0.0 #764

Upgrade to Cucumber Expressions 3.0.0 #764

aslakhellesoy commented Feb 17, 2017 •

edited

Loading

jbpros left a comment

jbpros Feb 17, 2017

aslakhellesoy Feb 17, 2017

jbpros Feb 17, 2017

charlierudolph Feb 17, 2017

mattbrunetti Feb 17, 2017

charlierudolph Feb 17, 2017

aslakhellesoy Feb 18, 2017

charlierudolph Feb 17, 2017

aslakhellesoy Feb 17, 2017

aslakhellesoy Feb 18, 2017

charlierudolph Feb 17, 2017

aslakhellesoy Feb 18, 2017

aslakhellesoy commented Feb 17, 2017

charlierudolph commented Feb 19, 2017

aslakhellesoy commented Feb 26, 2017

aslakhellesoy commented Mar 1, 2017

charlierudolph commented Mar 1, 2017

aslakhellesoy commented Mar 2, 2017

aslakhellesoy commented Mar 9, 2017

charlierudolph left a comment

charlierudolph Mar 9, 2017

aslakhellesoy Mar 9, 2017

charlierudolph Mar 9, 2017

charlierudolph Mar 9, 2017

charlierudolph Mar 9, 2017

lock bot commented Oct 25, 2018

Upgrade to Cucumber Expressions 3.0.0 #764

Upgrade to Cucumber Expressions 3.0.0 #764

Conversation

aslakhellesoy commented Feb 17, 2017 • edited Loading

jbpros left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aslakhellesoy commented Feb 17, 2017

charlierudolph commented Feb 19, 2017

aslakhellesoy commented Feb 26, 2017

aslakhellesoy commented Mar 1, 2017

charlierudolph commented Mar 1, 2017

aslakhellesoy commented Mar 2, 2017

aslakhellesoy commented Mar 9, 2017

charlierudolph left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lock bot commented Oct 25, 2018

aslakhellesoy commented Feb 17, 2017 •

edited

Loading