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.

Sign up for GitHub

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

Jump to bottom

lib: remove excess indentation #14090

Closed
wants to merge 1 commit into from
Closed

lib: remove excess indentation #14090

wants to merge 1 commit into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Jul 5, 2017

In anticipation of stricter linting for indentation, remove instances of
extra indentation that will be flagged by the new rules.

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

lib tools

@Trott Trott added lib / src Issues and PRs related to general changes in the lib or src directory. tools Issues and PRs related to the tools directory. labels Jul 5, 2017
@nodejs-github-bot nodejs-github-bot added the lib / src Issues and PRs related to general changes in the lib or src directory. label Jul 5, 2017
@Trott
Copy link
Member Author

Trott commented Jul 5, 2017

CI: https://ci.nodejs.org/job/node-test-pull-request/8990/

BridgeAR
BridgeAR requested changes Jul 5, 2017
View reviewed changes
lib/async_hooks.js
@@ -71,8 +71,7 @@ function fatalError(e) {
Error.captureStackTrace(o, fatalError);
process._rawDebug(o.stack);
}
if (process.execArgv.some(
(e) => /^--abort[_-]on[_-]uncaught[_-]exception$/.test(e))) {
if (process.execArgv.some((e) => /^--abort[_-]on[_-]uncaught[_-]exception$/.test(e))) {
Copy link
Member

Choose a reason for hiding this comment

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

This seems to be above 80 chars.

Copy link
Member Author

Choose a reason for hiding this comment

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

As of May, the linter doesn't enforce the line-length rule on lines with regular expression literals. See #12807 for rationale. (Working as intended here too, IMO. I find this much more readable than either the current formatting or any other reasonable formatting I was able to come up with.)

Copy link
Member Author

Choose a reason for hiding this comment

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

I suppose one way to keep it readable and under 80 chars is to assign the regular expression to a variable on the previous line and use the variable here. I would prefer to avoid that because it means this PR is more than formatting changes. I'm not sure how reasonable that is of me, though. :-D

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the info!

I think it would still be nice to move the expression up in it's own variable though, since it prevents the RegExp from being recreated multiple times.

Copy link
Member Author

@Trott Trott Jul 5, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

This is inside fatalError() so I don't think RegExp re-creation is an issue. If the RegExp passes, process.abort() is called. If the RegExp fails, process.exit() is called.

@Trott
lib: remove excess indentation
2c385d9 
In anticipation of stricter linting for indentation, remove instances of
extra indentation that will be flagged by the new rules.
@Trott
Copy link
Member Author

Trott commented Jul 7, 2017

CI: https://ci.nodejs.org/job/node-test-pull-request/9029/

@Trott
Copy link
Member Author

Trott commented Jul 7, 2017

Landed in 0d22858

@Trott Trott closed this Jul 7, 2017
Trott added a commit to Trott/io.js that referenced this pull request Jul 7, 2017
@Trott
lib: remove excess indentation
0d22858 
In anticipation of stricter linting for indentation, remove instances of
extra indentation that will be flagged by the new rules.

PR-URL: nodejs#14090
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
addaleax pushed a commit that referenced this pull request Jul 11, 2017
@Trott @addaleax
lib: remove excess indentation
7e86c5c 
In anticipation of stricter linting for indentation, remove instances of
extra indentation that will be flagged by the new rules.

PR-URL: #14090
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@addaleax addaleax mentioned this pull request Jul 11, 2017
Merged
addaleax pushed a commit that referenced this pull request Jul 18, 2017
@Trott @addaleax
lib: remove excess indentation
a0866b6 
In anticipation of stricter linting for indentation, remove instances of
extra indentation that will be flagged by the new rules.

PR-URL: #14090
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Fishrock123 pushed a commit that referenced this pull request Jul 19, 2017
@Trott @Fishrock123
lib: remove excess indentation
d9c3cca 
In anticipation of stricter linting for indentation, remove instances of
extra indentation that will be flagged by the new rules.

PR-URL: #14090
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Aug 15, 2017
Closed
7 tasks
Trott added a commit to Trott/io.js that referenced this pull request Aug 15, 2017
@Trott
lib: remove excess indentation
dfe9894 
In anticipation of stricter linting for indentation, remove instances of
extra indentation that will be flagged by the new rules.

PR-URL: nodejs#14090
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
MylesBorins pushed a commit that referenced this pull request Aug 16, 2017
@Trott @MylesBorins
lib: remove excess indentation
5e6c711 
In anticipation of stricter linting for indentation, remove instances of
extra indentation that will be flagged by the new rules.

Backport-PR-URL: #14835
PR-URL: #14090
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Aug 16, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Sep 5, 2017
@Trott @MylesBorins
lib: remove excess indentation
4dabeee 
In anticipation of stricter linting for indentation, remove instances of
extra indentation that will be flagged by the new rules.

Backport-PR-URL: #14835
PR-URL: #14090
Reviewed-By: Benjamin Gruenbaum <[email protected]>
Reviewed-By: Tobias Nießen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
This was referenced Sep 5, 2017
Closed
Closed
Closed
This was referenced Sep 6, 2017
Closed
Closed
@Trott Trott deleted the rm-extra-indentation branch January 13, 2022 22:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benjamingr benjamingr benjamingr approved these changes

@lpinca lpinca lpinca approved these changes

@tniessen tniessen tniessen approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

Assignees
No one assigned
Labels
lib / src Issues and PRs related to general changes in the lib or src directory. tools Issues and PRs related to the tools directory.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@Trott @benjamingr @lpinca @tniessen @BridgeAR @MylesBorins @nodejs-github-bot