Skip to content

Conversation

@Trott
Copy link
Member

In preparation for stricter indentation linting and to increase code
clarity, update indentation for ternaries in lib.

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

lib tools

@TrottTrott 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 15, 2017
@nodejs-github-botnodejs-github-bot added events Issues and PRs related to the events subsystem / EventEmitter. net Issues and PRs related to the net subsystem. url Issues and PRs related to the legacy built-in url module. labels Jul 15, 2017
@addaleax
Copy link
Member

There’s a typo in the commit message ;)

lib/url.js Outdated
Copy link
Member

Choose a reason for hiding this comment

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

This doesn’t need to be a ternary anyway, right? Just result.host && result.host.indexOf('@') > 0 && result.host.split('@') works as well?

Copy link
MemberAuthor

Choose a reason for hiding this comment

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

Good idea! I don't think there's much of a chance this is being done as a ternary because it's more performant than the alternative approach you're suggesting here. But that's usually behind my reluctance to alter anything other than whitespace in these things. :-D

@Trott
Copy link
MemberAuthor

There’s a typo in the commit message ;)

Fixed. Thanks!

@TrottTrottforce-pushed the indent-ternaries-in-lib branch from 379141d to 77b868aCompareJuly 15, 2017 04:07
@TrottTrott changed the title lib: update indentaion of ternarieslib: update indentation of ternariesJul 15, 2017
@TrottTrottforce-pushed the indent-ternaries-in-lib branch from 77b868a to c915e9fCompareJuly 15, 2017 06:21
In preparation for stricter indentation linting and to increase code clarity, update indentation for ternaries in lib.
@Trott
Copy link
MemberAuthor

Trott added a commit to Trott/io.js that referenced this pull request Jul 18, 2017
In preparation for stricter indentation linting and to increase code clarity, update indentation for ternaries in lib. PR-URL: nodejs#14247 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: James M Snell <[email protected]>
@Trott
Copy link
MemberAuthor

Landed in d14c132

@TrottTrott closed this Jul 18, 2017
@addaleaxaddaleax mentioned this pull request Jul 18, 2017
addaleax pushed a commit that referenced this pull request Jul 18, 2017
In preparation for stricter indentation linting and to increase code clarity, update indentation for ternaries in lib. PR-URL: #14247 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: James M Snell <[email protected]>
Fishrock123 pushed a commit that referenced this pull request Jul 19, 2017
In preparation for stricter indentation linting and to increase code clarity, update indentation for ternaries in lib. PR-URL: #14247 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: James M Snell <[email protected]>
Trott added a commit to Trott/io.js that referenced this pull request Aug 15, 2017
In preparation for stricter indentation linting and to increase code clarity, update indentation for ternaries in lib. PR-URL: nodejs#14247 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Aug 16, 2017
In preparation for stricter indentation linting and to increase code clarity, update indentation for ternaries in lib. Backport-PR-URL: #14835 PR-URL: #14247 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: James M Snell <[email protected]>
@MylesBorinsMylesBorins mentioned this pull request Aug 16, 2017
MylesBorins pushed a commit that referenced this pull request Sep 5, 2017
In preparation for stricter indentation linting and to increase code clarity, update indentation for ternaries in lib. Backport-PR-URL: #14835 PR-URL: #14247 Reviewed-By: Anna Henningsen <[email protected]> Reviewed-By: Luigi Pinca <[email protected]> Reviewed-By: Joyee Cheung <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Gibson Fahnestock <[email protected]> Reviewed-By: Tobias Nießen <[email protected]> Reviewed-By: James M Snell <[email protected]>
@TrottTrott deleted the indent-ternaries-in-lib branch January 13, 2022 22:46
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Labels

eventsIssues and PRs related to the events subsystem / EventEmitter.lib / srcIssues and PRs related to general changes in the lib or src directory.netIssues and PRs related to the net subsystem.toolsIssues and PRs related to the tools directory.urlIssues and PRs related to the legacy built-in url module.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants

@Trott@addaleax@jasnell@lpinca@targos@tniessen@joyeecheung@gibfahn@MylesBorins@nodejs-github-bot