WIP: make helper-grammar-regex-collection more readable #282

josephfrazier · 2017-02-15T03:26:09Z

This uses XRegExp to allow the regular expressions in
helper-grammar-regex-collection to:

be written in a whitespace-agnostic format
to share common subpatterns like ['"]([^'"\s]+)['"]
to only capture named groups, to avoid (?: everywhere

It takes advantage of template literals to allow the patterns to be
written as multi-line strings, but without having to double-escape
special characters.

@stefanbuck, what do you think? In my opinion, this makes the regex
syntax a bit more verbose, but much easier to read and modify.

http://xregexp.com/flags/#explicitCapture

https://gist.github.com/slevithan/4222600

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Template_literals#Tagged_template_literals

XXX note that moving 'x' makes tests fail. Not sure why

Note that XRegExp.build automatically wraps it into a group when used as a subpattern

It can match the empty string, and isn't captured anyway

This cuts dist/app.js from 724K back down to 580K

stefanbuck · 2017-02-15T06:10:23Z

This looks awesome, well done! I'll take a closer look later.

stefanbuck

This is amazing. I really like the subpattern concept, so genius. Added two minor comments, besides LGTM

stefanbuck · 2017-02-15T20:51:49Z

packages/helper-grammar-regex-collection/index.js

+const subpatterns = {};
+const regex = regexBuilder(subpatterns);
+
+subpatterns.captureQuotedWord = regex`


It feels a bit odd to me to modify subpatterns after passing it to regexBuilder. Just my personal preference.

I just pushed up a refactor I had been working on, that addresses this by allowing normal template literal interpolation to be used, so that subpatterns isn't exposed outside of the regex tag function.

For what it's worth, this refactor was also done with the idea that I might eventually contribute it back to XRegExp. See slevithan/xregexp#103

Nice refactoring, looks damn good 👍 It's great to see your contribution to other project as well. Hopefully they will respond soon.

Thanks! Once we sort out the naming question, I'll merge it.

If XRegExp later decides to incorporate the tag function, we'll be able to use it directly instead. Otherwise, I could always publish a separate module to make the tag more generally available.

stefanbuck · 2017-02-15T20:57:39Z

packages/helper-grammar-regex-collection/index.js

-const RUST_CRATE = /(?:extern crate|use) ([^:; ]+)/g;
-const PYTHON_IMPORT = /^\s*(?:import|from)\s([^\s]*)/gm;
+const subpatterns = {};
+const regex = regexBuilder(subpatterns);


The result from regexBuilder is actually the regexBuilder, isn't it? Therefore I would prefer to rename regex to regexBuilder and regexBuilder to some different.

Yeah, the naming is a little strange here. My perspective on it is that regex is a function that takes a string and returns a RegExp, much like its constructor: RegExp('my pattern'), so the regex name makes sense to me.

However, regexBuilder is still a little awkward. While it does build the regex function, it still sounds like it might do the same thing as regex. Besides, I discovered there's already another regexBuilder in the codebase (lib/plugins/helper/regex-builder.js). I not sure what a better name would be, though. Some ideas:

regexTagBuilder

regexTagFunctionBuilder

What if we just get rid of of the regexBuilder function, and have regex use the same flags every time (since the code calling it doesn't actually need to use different flags)? I just pushed up a commit showing what it could look like: a3a25ad?w=true

Look good and your explanation behind naming it regex makes sense to me as well.

Thanks, I just merged in the changes! I'm still working on learning why this is necessary, but I wanted to go ahead and land the rest of it, since it would conflict with future changes to the regular expressions.

EDIT: I reported a bug corresponding to the code linked above: slevithan/xregexp#162

Since we never actually use any other combination of flags, we can just export `regex` with the flags already set, rather than exporting `regexBuilder`. See conversation here: https://github.com/OctoLinker/browser-extension/pull/282/files/54d73ecb030eb4a3a3f29a5a4ccf331f4ecd0caa#r101380958

josephfrazier added 25 commits February 14, 2017 22:26

wip: install xregexp

1a03d49

wip: xregexp.build

bd14953

wip: captureQuotedDep

d4dc638

wip: importMembers

ade0a96

wip: free-spacing

e48d092

wip: multiline

196c3c9

wip: explicit capture

4d8ae42

http://xregexp.com/flags/#explicitCapture

wip: add whitespace to help readability

98ef859

wip: comment

d35d874

wip: oops, can't use named references with findAndReplaceDOMText

664a2e4

wip: raw

c921ad0

https://gist.github.com/slevithan/4222600

fixup! wip: oops, can't use named references with findAndReplaceDOMText

614de33

fixup! wip: raw

8c66630

wip: use fancy tagged template literal

cb0f9f6

https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Template_literals#Tagged_template_literals

wip: move 'n' flag outside pattern

dcc92ab

XXX note that moving 'x' makes tests fail. Not sure why

wip: add from subpattern

abcc5dc

wip: use regex to build subpatterns

a0ca3bd

wip: make it easier to debug regex

0d0aa67

wip: comment captureQuoteDep regex

7c7a99c

wip: simplify from subpattern

f051b50

Note that XRegExp.build automatically wraps it into a group when used as a subpattern

wip: rename captureQuotedDep to captureQuotedWord

eda9283

wip: remove useless suffix from REQUIRE regex

e413ae0

It can match the empty string, and isn't captured anyway

wip: move regexBuilder into separate file

0c64a47

wip: regexBuilder only imports core xregexp and build

e6d273b

This cuts dist/app.js from 724K back down to 580K

wip: comment regexBuilder.js

54d73ec

josephfrazier force-pushed the readable-regex branch from 4a61a56 to 54d73ec Compare February 15, 2017 03:27

josephfrazier added Do not merge to-discuss labels Feb 15, 2017

josephfrazier requested a review from stefanbuck February 15, 2017 03:27

josephfrazier self-assigned this Feb 15, 2017

josephfrazier added 9 commits February 15, 2017 01:49

wip: regexBuilder takes an object argument

56094e5

wip: use actual interpolation in template literals

e3f946b

wip: make substitutions and literals same length to avoid conditional

515a0f8

wip: explain 'x' flag treatment, simplify

9dfa95e

wip: explain substitutions.push

174013d

wip: use zip to make literals/subsititions iteration elegant

e7da256

wip: use forEach instead of reduce

ed79deb

wip: remove superfluous modeModifiers

a2046b2

wip: extract interpolate function

2c2d663

stefanbuck approved these changes Feb 15, 2017

View reviewed changes

josephfrazier mentioned this pull request Feb 15, 2017

Implement tagged template string slevithan/xregexp#103

Closed

josephfrazier added 2 commits February 16, 2017 16:28

wip: fix regex.js usage example

131d3a2

josephfrazier removed Do not merge to-discuss labels Feb 17, 2017

josephfrazier merged commit 204d3d2 into OctoLinker:master Feb 17, 2017

josephfrazier deleted the readable-regex branch February 17, 2017 16:00

stefanbuck mentioned this pull request Feb 22, 2017

[feature request] css @import linking #290

Closed

josephfrazier mentioned this pull request Mar 5, 2017

Add css import support #304

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: make helper-grammar-regex-collection more readable #282

WIP: make helper-grammar-regex-collection more readable #282

josephfrazier commented Feb 15, 2017 •

edited

stefanbuck commented Feb 15, 2017 •

edited

stefanbuck left a comment

stefanbuck Feb 15, 2017

josephfrazier Feb 15, 2017

stefanbuck Feb 16, 2017

josephfrazier Feb 16, 2017

stefanbuck Feb 15, 2017

josephfrazier Feb 15, 2017

josephfrazier Feb 16, 2017 •

edited

stefanbuck Feb 16, 2017

josephfrazier Feb 17, 2017 •

edited

WIP: make helper-grammar-regex-collection more readable #282

WIP: make helper-grammar-regex-collection more readable #282

Conversation

josephfrazier commented Feb 15, 2017 • edited

stefanbuck commented Feb 15, 2017 • edited

stefanbuck left a comment

Choose a reason for hiding this comment

stefanbuck Feb 15, 2017

Choose a reason for hiding this comment

josephfrazier Feb 15, 2017

Choose a reason for hiding this comment

stefanbuck Feb 16, 2017

Choose a reason for hiding this comment

josephfrazier Feb 16, 2017

Choose a reason for hiding this comment

stefanbuck Feb 15, 2017

Choose a reason for hiding this comment

josephfrazier Feb 15, 2017

Choose a reason for hiding this comment

josephfrazier Feb 16, 2017 • edited

Choose a reason for hiding this comment

stefanbuck Feb 16, 2017

Choose a reason for hiding this comment

josephfrazier Feb 17, 2017 • edited

Choose a reason for hiding this comment

josephfrazier commented Feb 15, 2017 •

edited

stefanbuck commented Feb 15, 2017 •

edited

josephfrazier Feb 16, 2017 •

edited

josephfrazier Feb 17, 2017 •

edited