Skip to content

Remove 'g' flag from RegExp conversions#750

Merged
tagliala merged 1 commit intoDavyJonesLocker:masterfrom
jaynetics:remove_reg_exp_g_flag
Oct 8, 2018
Merged

Remove 'g' flag from RegExp conversions#750
tagliala merged 1 commit intoDavyJonesLocker:masterfrom
jaynetics:remove_reg_exp_g_flag

Conversation

@jaynetics
Copy link
Copy Markdown
Contributor

While looking into jaynetics/js_regex#8, I noticed that you manually added the "g" flag back in when you upgraded to js_regex 3.0.0.

The "g" flag is only useful for substring replacement or counting. It can safely be removed for your use case.

It is actually better to remove it. It might cause bugs if the JS ever changes in a certain way (involving re-use of a RegExp as described in jaynetics/js_regex#5).

Cheers!

@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage remained the same at 100.0% when pulling dc4fbe3 on janosch-x:remove_reg_exp_g_flag into 646ab34 on DavyJonesLocker:master.

1 similar comment
@coveralls
Copy link
Copy Markdown

Coverage Status

Coverage remained the same at 100.0% when pulling dc4fbe3 on janosch-x:remove_reg_exp_g_flag into 646ab34 on DavyJonesLocker:master.

@tagliala
Copy link
Copy Markdown
Contributor

tagliala commented Oct 7, 2018

Thanks.

Could this commit represent a breaking change in some use cases?

I'm asking because I don't know if I should bump up the major version number

@jaynetics
Copy link
Copy Markdown
Contributor Author

It can't affect any validation so I'm not even sure it warrants a Changelog entry.

The only breaking change would be if someone uses the generated JSON for other purposes (e.g. substring replacement).

@tagliala tagliala merged commit bc08aa1 into DavyJonesLocker:master Oct 8, 2018
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.

3 participants