Add XRegExp.tag in `build` plugin for tagged template string construction #180

josephfrazier · 2017-04-17T17:50:14Z

Fixes #103

slevithan

Awesome to have this in XRegExp. Thanks @josephfrazier.

slevithan · 2017-04-25T05:21:38Z

src/addons/build.js

+     *
+     * [1]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Template_literals#Tagged_template_literals
+     * [2]: https://github.com/slevithan/xregexp/issues/103
+     */


Please add an @example of using this to the comment

slevithan · 2017-04-25T05:21:53Z

src/addons/build.js

+     * [1]: https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Template_literals#Tagged_template_literals
+     * [2]: https://github.com/slevithan/xregexp/issues/103
+     */
+    XRegExp.tag = function (flags) {


nit: remove space after function to match style used by the rest of the library

slevithan · 2017-04-25T05:22:11Z

src/addons/build.js

+     * [2]: https://github.com/slevithan/xregexp/issues/103
+     */
+    XRegExp.tag = function (flags) {
+        return function tag (literals /*, ...substitutions */) {


nit: remove space after tag. probably just remove the function name tag as well unless you feel this improves it.

slevithan · 2017-04-25T05:22:43Z

src/addons/build.js

+     */
+    XRegExp.tag = function (flags) {
+        return function tag (literals /*, ...substitutions */) {
+            var substitutions = [].slice.call(arguments, 1);


Will you be converting this to ES2015 with ...substitutions and getting rid of this?

Yes, I just wanted to make it clear for now. I'll definitely follow up with a PR that uses rest parameters across the codebase.

slevithan · 2017-04-25T05:23:54Z

src/addons/build.js

+            return XRegExp.build(pattern, subpatterns, flags);
+        };
+
+        function interpolate (substitution) {


Please move interpolate and embedSubpatternAfter out of and above XRegExp.tag, to the same level as asXRegExp. And remove the spaces after the function names. :)

slevithan · 2017-04-25T05:25:00Z

src/addons/build.js

+    XRegExp.tag = function (flags) {
+        return function tag (literals /*, ...substitutions */) {
+            var substitutions = [].slice.call(arguments, 1);
+            var subpatterns = substitutions.concat('').map(interpolate);


Feels a little inelegant to always insert an additional unnecessary empty string substitution at the end of pattern (enabled here by concat-ing an empty string). I'm okay with this as is but if there's a simple way to avoid this, that might be better.

slevithan · 2017-04-25T05:25:15Z

tests/spec/s-addons-build.js

@@ -1,5 +1,81 @@
 describe('XRegExp.build addon:', function() {

+    describe('XRegExp.tag()', function() {


These tests are great--thanks!

Also add a test that verifies the example. See slevithan#180 (comment)

…p.tag See slevithan#180 (comment)

See: * slevithan#180 (comment) * slevithan#180 (comment)

@example

There were a couple problems here: 1. Using the index as a subpattern name clashed with capturing groups, so I prepended 'subpattern' to the index. 2. XRegExp.tag does not infer capturing group names like XRegExp.build, so I changed the @example and test accordingly.

Since numbered groups can get hard to keep track of, show a better example instead.

This will make it easier to automatically enforce the pre-existing code style of the project, using [eslint]. It was done by running: npm install --save-dev eslint ./node_modules/.bin/eslint --init [eslint]: https://github.com/eslint/eslint/ This script interactively generates an eslint configuration (see [here]). Here's how I answered its questions: * Choosing `Inspect your JavaScript file(s)` * Entering `src` as the files to inspect * Choosing a JavaScript config file format * Answering yes to ES6 features/classes * Choosing both Node/Browser as environment * Answering yes to CommonJS * Answering no to JSX/React [here]: http://devnull.guru/adding-eslint-to-your-project-is-easier-than-ever/ After that, I added the `lint` script to package.json, and manually changed the `space-before-function-paren` to: "space-before-function-paren": [ "error", "never" ], to enforce the rule mentioned here: * slevithan#180 (comment) * slevithan#180 (comment)

embedSubpatternAfter has access to the index of the element it's operating on, so we can just check it to see whether the subpattern is present. See slevithan#180 (comment)

josephfrazier · 2017-04-25T07:01:08Z

Thanks for the review! I just pushed up a batch of commits that should address your comments. The one about adding an example actually prompted me to add another test and discover/fix a bug. Let me know what you think!

slevithan · 2017-04-25T21:15:09Z

tests/spec/s-addons-build.js

+            var minutes = /^[0-5][0-9]$/;
+            var time = XRegExp.tag('x')`^ ${hours} (${minutes}) $`
+            expect(time.test('10:59')).toBe(true);
+            expect(XRegExp.exec('10:59', time).minutes).toEqual('59');


Nit: I'd prefer to use toBe wherever strict equality is appropriate, even if just for consistency (toBe is used one line up).

slevithan · 2017-04-25T21:21:16Z

src/addons/build.js

@@ -83,12 +88,12 @@ module.exports = function(XRegExp) {
     * var minutes = /^[0-5][0-9]$/;
     * var time = XRegExp.tag('x')`^ ${hours} (${minutes}) $`
     * time.test('10:59'); // -> true
-     * XRegExp.exec('10:59', time).minutes; // -> '59'
+     * XRegExp.exec('10:59', time)[1]; // -> '59'


Maybe just remove this line and the capturing group around ${minutes} since this is no longer showing an additional feature.

Edit: Nevermind. I see you modified this in a valuable way (to show an explicitly named capturing group) in a following diff.

Also add a test that verifies the example. See #180 (comment)

…p.tag See #180 (comment)

See: * #180 (comment) * #180 (comment)

This will make it easier to automatically enforce the pre-existing code style of the project, using [eslint]. It was done by running: npm install --save-dev eslint ./node_modules/.bin/eslint --init [eslint]: https://github.com/eslint/eslint/ This script interactively generates an eslint configuration (see [here]). Here's how I answered its questions: * Choosing `Inspect your JavaScript file(s)` * Entering `src` as the files to inspect * Choosing a JavaScript config file format * Answering yes to ES6 features/classes * Choosing both Node/Browser as environment * Answering yes to CommonJS * Answering no to JSX/React [here]: http://devnull.guru/adding-eslint-to-your-project-is-easier-than-ever/ After that, I added the `lint` script to package.json, and manually changed the `space-before-function-paren` to: "space-before-function-paren": [ "error", "never" ], to enforce the rule mentioned here: * #180 (comment) * #180 (comment)

As mentioned in slevithan#180 (comment) and slevithan#108 (comment) This was done with the help of [lebab], using `lebeb --replace src --transform arg-rest`. [lebab]: https://github.com/lebab/lebab

As mentioned in #180 (comment) and #108 (comment) This was done with the help of [lebab], using `lebeb --replace src --transform arg-rest`. [lebab]: https://github.com/lebab/lebab

Also add a test that verifies the example. See slevithan/xregexp#180 (comment)

…p.tag See slevithan/xregexp#180 (comment)

See: * slevithan/xregexp#180 (comment) * slevithan/xregexp#180 (comment)

embedSubpatternAfter has access to the index of the element it's operating on, so we can just check it to see whether the subpattern is present. See slevithan/xregexp#180 (comment)

This will make it easier to automatically enforce the pre-existing code style of the project, using [eslint]. It was done by running: npm install --save-dev eslint ./node_modules/.bin/eslint --init [eslint]: https://github.com/eslint/eslint/ This script interactively generates an eslint configuration (see [here]). Here's how I answered its questions: * Choosing `Inspect your JavaScript file(s)` * Entering `src` as the files to inspect * Choosing a JavaScript config file format * Answering yes to ES6 features/classes * Choosing both Node/Browser as environment * Answering yes to CommonJS * Answering no to JSX/React [here]: http://devnull.guru/adding-eslint-to-your-project-is-easier-than-ever/ After that, I added the `lint` script to package.json, and manually changed the `space-before-function-paren` to: "space-before-function-paren": [ "error", "never" ], to enforce the rule mentioned here: * slevithan/xregexp#180 (comment) * slevithan/xregexp#180 (comment)

As mentioned in slevithan/xregexp#180 (comment) and slevithan/xregexp#108 (comment) This was done with the help of [lebab], using `lebeb --replace src --transform arg-rest`. [lebab]: https://github.com/lebab/lebab

Also add a test that verifies the example. See slevithan/xregexp#180 (comment)

…p.tag See slevithan/xregexp#180 (comment)

See: * slevithan/xregexp#180 (comment) * slevithan/xregexp#180 (comment)

embedSubpatternAfter has access to the index of the element it's operating on, so we can just check it to see whether the subpattern is present. See slevithan/xregexp#180 (comment)

This will make it easier to automatically enforce the pre-existing code style of the project, using [eslint]. It was done by running: npm install --save-dev eslint ./node_modules/.bin/eslint --init [eslint]: https://github.com/eslint/eslint/ This script interactively generates an eslint configuration (see [here]). Here's how I answered its questions: * Choosing `Inspect your JavaScript file(s)` * Entering `src` as the files to inspect * Choosing a JavaScript config file format * Answering yes to ES6 features/classes * Choosing both Node/Browser as environment * Answering yes to CommonJS * Answering no to JSX/React [here]: http://devnull.guru/adding-eslint-to-your-project-is-easier-than-ever/ After that, I added the `lint` script to package.json, and manually changed the `space-before-function-paren` to: "space-before-function-paren": [ "error", "never" ], to enforce the rule mentioned here: * slevithan/xregexp#180 (comment) * slevithan/xregexp#180 (comment)

As mentioned in slevithan/xregexp#180 (comment) and slevithan/xregexp#108 (comment) This was done with the help of [lebab], using `lebeb --replace src --transform arg-rest`. [lebab]: https://github.com/lebab/lebab

josephfrazier added 2 commits April 17, 2017 13:48

Add XRegExp.tag in build plugin for tagged template string construc…

1cfb317

…tion Fixes slevithan#103

build

094f990

josephfrazier mentioned this pull request Apr 19, 2017

Use babel with babel-preset-env to compile src/ files to lib/ #174

Merged

slevithan approved these changes Apr 25, 2017

View reviewed changes

josephfrazier added 5 commits April 25, 2017 01:53

Add @example to XRegExp.tag

5c3d389

Also add a test that verifies the example. See slevithan#180 (comment)

Move interpolate and embedSubpatternAfter out of and above XRegEx…

6b1e559

…p.tag See slevithan#180 (comment)

Remove extraneous function names/spaces

8e3f715

See: * slevithan#180 (comment) * slevithan#180 (comment)

Use named group in XRegExp.tag @example/test

202a328

Since numbered groups can get hard to keep track of, show a better example instead.

josephfrazier mentioned this pull request Apr 25, 2017

Add eslint to enforce code style #184

Merged

Don't append empty substitution in XRegExp.tag

48c3eda

embedSubpatternAfter has access to the index of the element it's operating on, so we can just check it to see whether the subpattern is present. See slevithan#180 (comment)

slevithan reviewed Apr 25, 2017

View reviewed changes

slevithan merged commit 81d36fe into slevithan:master Apr 25, 2017

slevithan pushed a commit that referenced this pull request Apr 25, 2017

Add @example to XRegExp.tag

7fdc10d

Also add a test that verifies the example. See #180 (comment)

slevithan pushed a commit that referenced this pull request Apr 25, 2017

Move interpolate and embedSubpatternAfter out of and above XRegEx…

9fb687d

…p.tag See #180 (comment)

slevithan pushed a commit that referenced this pull request Apr 25, 2017

Remove extraneous function names/spaces

5f81da2

See: * #180 (comment) * #180 (comment)

josephfrazier mentioned this pull request Apr 25, 2017

Use ES2015 rest parameters in src/ #186

Merged

slevithan mentioned this pull request Apr 27, 2017

Fix interpolation within character classes for XRegExp.build/tag #192

Open

josephfrazier deleted the template branch April 30, 2017 23:42

speecyy added a commit to speecyy/xregexp that referenced this pull request Jan 6, 2018

Add @example to XRegExp.tag

81932e8

Also add a test that verifies the example. See slevithan/xregexp#180 (comment)

speecyy added a commit to speecyy/xregexp that referenced this pull request Jan 6, 2018

Move interpolate and embedSubpatternAfter out of and above XRegEx…

aff1a5a

…p.tag See slevithan/xregexp#180 (comment)

speecyy added a commit to speecyy/xregexp that referenced this pull request Jan 6, 2018

Remove extraneous function names/spaces

a421629

See: * slevithan/xregexp#180 (comment) * slevithan/xregexp#180 (comment)

3590212051 added a commit to 3590212051/POPmotion that referenced this pull request Jan 8, 2018

Add @example to XRegExp.tag

134ece0

Also add a test that verifies the example. See slevithan/xregexp#180 (comment)

3590212051 added a commit to 3590212051/POPmotion that referenced this pull request Jan 8, 2018

Move interpolate and embedSubpatternAfter out of and above XRegEx…

25a6524

…p.tag See slevithan/xregexp#180 (comment)

3590212051 added a commit to 3590212051/POPmotion that referenced this pull request Jan 8, 2018

Remove extraneous function names/spaces

26e84d4

See: * slevithan/xregexp#180 (comment) * slevithan/xregexp#180 (comment)

renovate bot mentioned this pull request Jun 6, 2021

fix(deps): update dependency xregexp to v5 AtomLinter/linter-write-good#97

Open

1 task

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add XRegExp.tag in `build` plugin for tagged template string construction #180

Add XRegExp.tag in `build` plugin for tagged template string construction #180

josephfrazier commented Apr 17, 2017

slevithan left a comment

slevithan Apr 25, 2017

slevithan Apr 25, 2017

slevithan Apr 25, 2017

slevithan Apr 25, 2017

josephfrazier Apr 25, 2017

slevithan Apr 25, 2017

slevithan Apr 25, 2017 •

edited

Loading

slevithan Apr 25, 2017

josephfrazier commented Apr 25, 2017

slevithan Apr 25, 2017

slevithan Apr 25, 2017 •

edited

Loading

		@@ -1,5 +1,81 @@
		describe('XRegExp.build addon:', function() {

		describe('XRegExp.tag()', function() {

Add XRegExp.tag in build plugin for tagged template string construction #180

Add XRegExp.tag in build plugin for tagged template string construction #180

Conversation

josephfrazier commented Apr 17, 2017

slevithan 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

slevithan Apr 25, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

josephfrazier commented Apr 25, 2017

Choose a reason for hiding this comment

slevithan Apr 25, 2017 • edited Loading

Choose a reason for hiding this comment

Add XRegExp.tag in `build` plugin for tagged template string construction #180

Add XRegExp.tag in `build` plugin for tagged template string construction #180

slevithan Apr 25, 2017 •

edited

Loading

slevithan Apr 25, 2017 •

edited

Loading