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: simplify no-unescaped-regexp-dot rule #14561

Closed
wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Aug 1, 2017

no-unescaped-regexp-dot ESLint custom rule contains feature detection
that is not needed on master or the v6.x-staging branch. The rule does
not exist in the v4.x-staging branch. Remove the unnecessary complexity.

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

tools

@nodejs-github-bot nodejs-github-bot added the tools Issues and PRs related to the tools directory. label Aug 1, 2017
@Trott
Copy link
Member Author

Trott commented Aug 1, 2017

//------------------------------------------------------------------------------
// Rule Definition
//------------------------------------------------------------------------------

module.exports = function(context) {
const sourceCode = context.getSourceCode();
const s = context.getSourceCode();
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the original, more descriptive variable name should be kept.

Copy link
Member Author

@Trott Trott Aug 1, 2017

Choose a reason for hiding this comment

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

Is it sufficiently more desirable to have a descriptive variable name that we're OK that it requires a line break in the object literal (to wrap at 80 chars due to the descriptive variable name) on line 20?

I'm OK either way. Just want to make sure the cost of the descriptive variable name is taken into account.

Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need to break the object format if you don't want to. You could temporarily store all or part of the loc in a separate variable to avoid that.

Copy link
Member Author

Choose a reason for hiding this comment

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

OK, restored the longer name etc.

no-unescaped-regexp-dot ESLint custom rule contains feature detection
that is not needed on master or the v6.x-staging branch. The rule does
not exist in the v4.x-staging branch. Remove the unnecessary complexity.
@Trott
Copy link
Member Author

Trott commented Aug 2, 2017

@jasnell
Copy link
Member

jasnell commented Aug 3, 2017

Failures in CI are a concern but are not related to this PR.

jasnell pushed a commit that referenced this pull request Aug 3, 2017
no-unescaped-regexp-dot ESLint custom rule contains feature detection
that is not needed on master or the v6.x-staging branch. The rule does
not exist in the v4.x-staging branch. Remove the unnecessary complexity.

PR-URL: #14561
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@jasnell
Copy link
Member

jasnell commented Aug 3, 2017

Landed in a92a5c1

@jasnell jasnell closed this Aug 3, 2017
addaleax pushed a commit that referenced this pull request Aug 7, 2017
no-unescaped-regexp-dot ESLint custom rule contains feature detection
that is not needed on master or the v6.x-staging branch. The rule does
not exist in the v4.x-staging branch. Remove the unnecessary complexity.

PR-URL: #14561
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@MylesBorins
Copy link
Contributor

no-unescaped-regexp-dot rule doesn't exist on LTS right now.

@Trott
Copy link
Member Author

Trott commented Aug 17, 2017

no-unescaped-regexp-dot rule doesn't exist on LTS right now.

@MylesBorins It was added in #11834 which has not yet been backported.

@Trott Trott deleted the feature-detect branch January 13, 2022 22:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants