cucumber-expressions: replace special characters #106

Merged
merged 2 commits into from Feb 16, 2017

Projects

None yet

2 participants

@charlierudolph
Member

resolves #103

Only the JS for now. After initial review, I'll update the java / ruby versions

@charlierudolph charlierudolph update
03daa1c
@charlierudolph charlierudolph requested a review from aslakhellesoy Jan 28, 2017
@aslakhellesoy aslakhellesoy cucumber-expressions-javascript: Simplify implementation, add tests
577c77c
@aslakhellesoy

LGTM! I simplified the logic - it can be done with a single statement.

Since several people are likely to work on the same branch in this repo, I think we need a naming standard for branches. I propose:

library[-language][-issue]-description - for example:

cucumber-expressions-103-replace-special-characters.

Or if this were a bugfix that only existed in the JavaScript implementation:

cucumber-expressions-javascript-103-replace-special-characters

@@ -63,7 +63,7 @@ describe(CucumberExpression.name, () => {
})
describe('RegExp special characters', () => {
- ['[', '\\', '^', '$', '?', '*', '+'].forEach((character) => {
+ ['\\', '[', ']', '^', '$', '.', '|', '?', '*', '+'].forEach((character) => {
@charlierudolph
charlierudolph Jan 30, 2017 Member

I don't think this change is needed. Moving the backslash to first is fine, but the addition of the closing square bracket, dot, and pipe are unneeded.

The dot and pipe are tested separately as these tests weren't failing before implement escaping.

The closing square bracket doesn't need to be tested as that too probably isn't failing and looking up regular expression special characters, it didn't appear that you need to escape the closing square bracket.

@aslakhellesoy
aslakhellesoy Jan 31, 2017 Member

You are right those extra tests are not needed. At first I was surprised some of the characters weren't part of the reused test. Then I was surprised to see some of the characters didn't require escaping. I think adding theses tests may reduce some of the surprises/puzzles in the future - consider it documentation.

@charlierudolph
charlierudolph Feb 3, 2017 Member

I'd rather comment why those tests were excluded then have them in if they aren't needed.

@aslakhellesoy
aslakhellesoy Feb 3, 2017 Member

Ok, fine with me.

@charlierudolph
Member

Also. @aslakhellesoy do you think this should be merged as is or should I update the ruby / java versions in this PR as well?

@aslakhellesoy
Member

We should update the ruby/java versions too, and ideally the shared tests (based on a manually copied text file for now)

@aslakhellesoy aslakhellesoy merged commit 577c77c into master Feb 16, 2017

6 checks passed

continuous-integration/gitbook/epub GitBook build "epub" succeeded
Details
continuous-integration/gitbook/json GitBook build "json" succeeded
Details
continuous-integration/gitbook/mobi GitBook build "mobi" succeeded
Details
continuous-integration/gitbook/pdf GitBook build "pdf" succeeded
Details
continuous-integration/gitbook/website GitBook build "website" succeeded
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment