Skip to content

Conversation

silentroach
Copy link
Contributor

@silentroach silentroach commented Apr 26, 2017

Regexp flags order isn't important, so it is better to use real regexp instead of its string representation.

/foo/mig.toString() = /foo/gim

Regexp flags order isn't important, so it is better to use real regexp
instead of its string representation.
valid: ['var foo = /foo/i'],
valid: [
'var foo = /foo/i',
'var foo = /foo/mig'
Copy link
Contributor Author

@silentroach silentroach Apr 26, 2017

Choose a reason for hiding this comment

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

without this fix it will be error here with optimized regexp /foo/gim

@BrainMaestro
Copy link
Owner

Hey @silentroach. Thanks for the PR.
Looks like this introduces a separate feature to handle and warn of invalid regexes which is great.

One question though, is preserving the flag order really necessary? Having all regex flags in a project follow a particular order might be a good thing right? On the other hand, it introduces a lot of unnecessary warnings.

@silentroach
Copy link
Contributor Author

Yep, the original reason for this pr is to remove this unneccessary warnings. In big project with tons of regexps it isn't great to replace all regexps with the same one, but with other flags order :}

@BrainMaestro
Copy link
Owner

Yeah I figured. One more thing, can you add a test for the case of parsing failing?

@silentroach
Copy link
Contributor Author

silentroach commented Apr 26, 2017

Here is an example of regexps that can't be parsed: / ([.|,])/g, /[\n|\t]/ig

But it is a regexp-tree bug, I will try to fix it later (or report). There is no reason to test it here though.

@BrainMaestro
Copy link
Owner

Okay. Thanks!

@BrainMaestro BrainMaestro merged commit 997a25d into BrainMaestro:master Apr 26, 2017
BrainMaestro pushed a commit that referenced this pull request Apr 26, 2017
Regexp flags order isn't important, so it is better to use real regexp
instead of its string representation.
@silentroach silentroach mentioned this pull request Apr 27, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants