Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Cleanup more (?:) from patterns #196

Merged
merged 9 commits into from
Jan 22, 2018
8 changes: 8 additions & 0 deletions src/xregexp.js
Original file line number Diff line number Diff line change
Expand Up @@ -238,6 +238,14 @@ function getContextualTokenSeparator(match, scope, flags) {
// No need to separate tokens if at the beginning or end of a group
match.input[match.index - 1] === '(' ||
match.input[match.index + match[0].length] === ')' ||
// No need to separate tokens if at the beginning of a non-capturing group
match.input.slice(match.index - 3, 3) === '(?:' ||
// No need to separate tokens if before or after a `|`
match.input[match.index - 1] === '|' ||
match.input[match.index + match[0].length] === '|' ||
// No need to separate tokens if at the beginning or end of the pattern
match.input[match.index - 1] === undefined ||
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you just check the value of match.index - 1 directly, without involving match.input?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, fixed in ee766c2 and 222eda9

match.input[match.index + match[0].length] === undefined ||
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ditto. Should probably be comparing to the length of match.input rather than referencing a specific character in the string.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also fixed in ee766c2

// Avoid separating tokens when the following token is a quantifier
isQuantifierNext(match.input, match.index + match[0].length, flags)
) {
Expand Down
15 changes: 15 additions & 0 deletions tests/spec/s-xregexp.js
Original file line number Diff line number Diff line change
Expand Up @@ -800,6 +800,21 @@ describe('XRegExp()', function() {
expect(XRegExp('(#\n.#\n)', 'x').source).toBe('(.)');
});

it('should not add atom separator (?:) at the beginning or end of non-capturing groups in simple cases', function() {
expect(XRegExp('(?: . )', 'x').source).toBe('(?:.)');
expect(XRegExp('(?:#\n.#\n)', 'x').source).toBe('(?:.)');
});

it('should not add atom separator (?:) at the beginning or end of the pattern in simple cases', function() {
expect(XRegExp(' ( . ) ', 'x').source).toBe('(.)');
expect(XRegExp(' (#\n.#\n) ', 'x').source).toBe('(.)');
});

it('should not add atom separator (?:) around | in simple cases', function() {
expect(XRegExp('( a | b )', 'x').source).toBe('(a|b)');
expect(XRegExp('(#\na#\n|#\nb#\n)', 'x').source).toBe('(a|b)');
});

it('should allow whitespace between ( and ? for special groups', function() {
expect(XRegExp('( ?:)', 'x').source).toBe('(?:)');
expect(XRegExp('( ?=)', 'x').source).toBe('(?=)');
Expand Down