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

tools: relax lint rule for regexps #12807

Closed
wants to merge 2 commits into from
Closed

Conversation

Trott
Copy link
Member

@Trott Trott commented May 3, 2017

Relax the rule for maximum line length in JS files if the line contains
a regular expression literal. This will avoid the need to convert a
regular expression literal into a RegExp constructor call broken across
multiple lines in order to satisfy the maximum line length rule. That
practice hampers readability.

In lib and test (but mostly test), replace RegExp constructors with regular expression literals where
possible.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

tools lib test

Trott added 2 commits May 2, 2017 23:11
Relax the rule for maximum line length in JS files if the line contains
a regular expression literal. This will avoid the need to convert a
regular expression literal into a RegExp constructor call broken across
multiple lines in order to satisfy the maximum line length rule. That
practice hampers readability.
Replace RegExp constructors with regular expression literals where
possible.
@Trott Trott added lib / src Issues and PRs related to general changes in the lib or src directory. test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory. labels May 3, 2017
@nodejs-github-bot nodejs-github-bot added http Issues or PRs related to the http subsystem. tools Issues and PRs related to the tools directory. labels May 3, 2017
@Trott
Copy link
Member Author

Trott commented May 3, 2017

Copy link
Contributor

@refack refack left a comment

Choose a reason for hiding this comment

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

👍 With both hands

@refack
Copy link
Contributor

refack commented May 3, 2017

When will JS get RegEx.VERBOSE 💚😞

@ChALkeR
Copy link
Member

ChALkeR commented May 4, 2017

This is great! The main purpose behind the max-len rule is to make code more readable (by forcing splitting the code into smaller logical chunks), but in the case of regexps it was doing the opposite. It's nice to see that fixed =).

@addaleax
Copy link
Member

addaleax commented May 5, 2017

Landed in f1d593c...6bcf65d

@addaleax addaleax closed this May 5, 2017
addaleax pushed a commit that referenced this pull request May 5, 2017
Relax the rule for maximum line length in JS files if the line contains
a regular expression literal. This will avoid the need to convert a
regular expression literal into a RegExp constructor call broken across
multiple lines in order to satisfy the maximum line length rule. That
practice hampers readability.

PR-URL: #12807
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
addaleax pushed a commit that referenced this pull request May 5, 2017
Replace RegExp constructors with regular expression literals where
possible.

PR-URL: #12807
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
anchnk pushed a commit to anchnk/node that referenced this pull request May 6, 2017
Relax the rule for maximum line length in JS files if the line contains
a regular expression literal. This will avoid the need to convert a
regular expression literal into a RegExp constructor call broken across
multiple lines in order to satisfy the maximum line length rule. That
practice hampers readability.

PR-URL: nodejs#12807
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
anchnk pushed a commit to anchnk/node that referenced this pull request May 6, 2017
Replace RegExp constructors with regular expression literals where
possible.

PR-URL: nodejs#12807
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@jasnell jasnell mentioned this pull request May 11, 2017
@gibfahn gibfahn mentioned this pull request Jun 15, 2017
3 tasks
@gibfahn
Copy link
Member

gibfahn commented Jun 18, 2017

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if no let me know or add the dont-land-on label.

Trott added a commit to Trott/io.js that referenced this pull request Jun 19, 2017
Relax the rule for maximum line length in JS files if the line contains
a regular expression literal. This will avoid the need to convert a
regular expression literal into a RegExp constructor call broken across
multiple lines in order to satisfy the maximum line length rule. That
practice hampers readability.

PR-URL: nodejs#12807
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
Trott added a commit to Trott/io.js that referenced this pull request Jun 19, 2017
Replace RegExp constructors with regular expression literals where
possible.

PR-URL: nodejs#12807
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@Trott
Copy link
Member Author

Trott commented Jun 19, 2017

@gibfahn Backport in #13776

gibfahn pushed a commit that referenced this pull request Jun 20, 2017
Relax the rule for maximum line length in JS files if the line contains
a regular expression literal. This will avoid the need to convert a
regular expression literal into a RegExp constructor call broken across
multiple lines in order to satisfy the maximum line length rule. That
practice hampers readability.

PR-URL: #12807
Backport-PR-URL: #13776
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
gibfahn pushed a commit that referenced this pull request Jun 20, 2017
Replace RegExp constructors with regular expression literals where
possible.

PR-URL: #12807
Backport-PR-URL: #13776
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@Trott Trott mentioned this pull request Jul 5, 2017
2 tasks
MylesBorins pushed a commit that referenced this pull request Jul 11, 2017
Relax the rule for maximum line length in JS files if the line contains
a regular expression literal. This will avoid the need to convert a
regular expression literal into a RegExp constructor call broken across
multiple lines in order to satisfy the maximum line length rule. That
practice hampers readability.

PR-URL: #12807
Backport-PR-URL: #13776
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
MylesBorins pushed a commit that referenced this pull request Jul 11, 2017
Replace RegExp constructors with regular expression literals where
possible.

PR-URL: #12807
Backport-PR-URL: #13776
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Vse Mozhet Byt <vsemozhetbyt@gmail.com>
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Yuta Hiroto <hello@about-hiroppy.com>
Reviewed-By: Сковорода Никита Андреевич <chalkerx@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: Anna Henningsen <anna@addaleax.net>
@MylesBorins MylesBorins mentioned this pull request Jul 18, 2017
@Trott Trott deleted the no-break-regexp branch April 11, 2021 22:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
http Issues or PRs related to the http subsystem. lib / src Issues and PRs related to general changes in the lib or src directory. test Issues and PRs related to the tests. tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants