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: use consistent indentation for ternaries #14078

Closed
wants to merge 5 commits into from
Closed

lib: use consistent indentation for ternaries #14078

wants to merge 5 commits into from

Conversation

Trott
Copy link
Member

@Trott Trott commented Jul 5, 2017

In anticipation of stricter linting for indentation issues, modify
ternary operators in lib that do not conform with the expected ESLint
settings.

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
mscdex
mscdex reviewed Jul 5, 2017
View reviewed changes
lib/cluster.js Outdated
module.exports = ('NODE_UNIQUE_ID' in process.env) ?
require('internal/cluster/child') :
require('internal/cluster/master');
module.exports = require(`internal/cluster/${
Copy link
Contributor

@mscdex mscdex 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.

I'm not particularly a fan of this multi-line inline substitution stuff. Can't we just set it to a variable and use the variable name instead?

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 agree. Yes, I did it, but I did not feel good about it. Will fix.

refack
refack reviewed Jul 5, 2017
View reviewed changes
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.

It's better than before, but could use a touch-up

lib/internal/bootstrap_node.js Outdated
@@ -276,8 +276,8 @@
get: function() {
if (!console) {
console = originalConsole === undefined ?
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe parans around the conditional?

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, will do.

lib/url.js Outdated
questionIdx :
hashIdx);
var firstIdx =
questionIdx !== -1 && (hashIdx === -1 || questionIdx < hashIdx) ?
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: assign the condition to a variable, and fit it in one line (useQuestionIdx)

Copy link
Member Author

Choose a reason for hiding this comment

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

Sure, done.

lib/url.js Outdated
@@ -585,8 +585,8 @@ Url.prototype.format = function format() {
host = auth + this.host;
} else if (this.hostname) {
host = auth + (this.hostname.indexOf(':') === -1 ?
this.hostname :
'[' + this.hostname + ']');
this.hostname :
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 I was once asked to explain this snippet in a job interview 😵
Maybe break after the open paran into an indented block:

host = auth + (
  this.hostname.indexOf(':') === -1 ?
  this.hostname :
  '[' + this.hostname + ']'
);

Copy link
Member Author

Choose a reason for hiding this comment

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

This one is tricky to fix that way because the custom align-multiline-assignment rule seems to conflict with the newer indent stuff. I'd been thinking it might be good to remove that custom rule anyway, but I don't want to do it ahead of time.

There are some other ways to make this code clearer, but they will require benchmarks to confirm that they don't adversely affect performance. Stay tuned.

Copy link
Contributor

Choose a reason for hiding this comment

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

Obviously my comment can be deffered, or replaced with a TODO

@Trott Trott added the blocked PRs that are blocked by other issues or PRs. label Jul 5, 2017
@Trott
Copy link
Member Author

Trott commented Jul 5, 2017

This will not lint cleanly without first removing the custom align-multiline-assignment rule. Marking as blocked.

@Trott Trott removed the blocked PRs that are blocked by other issues or PRs. label Jul 7, 2017
Trott added 5 commits July 6, 2017 22:47
@Trott
lib: use consistent indentation for ternaries
49336e1 
In anticipation of stricter linting for indentation issues, modify
ternary operators in lib that do not conform with the expected ESLint
settings.
@Trott
squash: address mscdex nit
88e6138
@Trott
squash: first nit from refack
c915b7e
@Trott
squash: second refack nit
5da6124
@Trott
squash: address third refack nit
a1f4bfb
@Trott
Copy link
Member Author

Trott commented Jul 7, 2017

This is now unblocked.

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

@Trott
Copy link
Member Author

Trott commented Jul 7, 2017

Landed in 85dacd6

@Trott Trott closed this Jul 7, 2017
Trott added a commit that referenced this pull request Jul 7, 2017
@Trott
lib: use consistent indentation for ternaries
85dacd6 
In anticipation of stricter linting for indentation issues, modify
ternary operators in lib that do not conform with the expected ESLint
settings.

PR-URL: #14078
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
addaleax pushed a commit that referenced this pull request Jul 11, 2017
@Trott @addaleax
lib: use consistent indentation for ternaries
55fca5f 
In anticipation of stricter linting for indentation issues, modify
ternary operators in lib that do not conform with the expected ESLint
settings.

PR-URL: #14078
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@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: use consistent indentation for ternaries
0764255 
In anticipation of stricter linting for indentation issues, modify
ternary operators in lib that do not conform with the expected ESLint
settings.

PR-URL: #14078
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Fishrock123 pushed a commit that referenced this pull request Jul 19, 2017
@Trott @Fishrock123
lib: use consistent indentation for ternaries
a1c342d 
In anticipation of stricter linting for indentation issues, modify
ternary operators in lib that do not conform with the expected ESLint
settings.

PR-URL: #14078
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@MylesBorins
Copy link
Contributor

Can you backport trott?

@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: use consistent indentation for ternaries
0e5f765 
In anticipation of stricter linting for indentation issues, modify
ternary operators in lib that do not conform with the expected ESLint
settings.

PR-URL: nodejs#14078
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
MylesBorins pushed a commit that referenced this pull request Aug 16, 2017
@Trott @MylesBorins
lib: use consistent indentation for ternaries
e8c0e0f 
In anticipation of stricter linting for indentation issues, modify
ternary operators in lib that do not conform with the expected ESLint
settings.

Backport-PR-URL: #14835
PR-URL: #14078
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
@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: use consistent indentation for ternaries
f20ed49 
In anticipation of stricter linting for indentation issues, modify
ternary operators in lib that do not conform with the expected ESLint
settings.

Backport-PR-URL: #14835
PR-URL: #14078
Reviewed-By: Refael Ackermann <refack@gmail.com>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
This was referenced Sep 5, 2017
Closed
Closed
Closed
Closed
Closed
@Trott Trott deleted the ternary-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

@mscdex mscdex mscdex left review comments

@refack refack refack approved these changes

@lpinca lpinca lpinca approved these changes

@cjihrig cjihrig cjihrig 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.
8 participants
@Trott @MylesBorins @mscdex @refack @lpinca @cjihrig @BridgeAR @nodejs-github-bot