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

path: inline conditions #38613

Closed
wants to merge 1 commit into from
Closed

path: inline conditions #38613

wants to merge 1 commit into from

Conversation

VoltrexKeyva
Copy link
Member

This condition can be inlined in the first if statement since if the path's length is 0, it'll be a empty string so we can return that as there's no need for an extra if statement.

@VoltrexKeyva
path: inline conditions
dee7116 
This condition can be inlined in the first `if` statement since if the `path`'s length is 0, it'll be a empty string so we can return that as there's no need for an extra `if` statement.
@github-actions github-actions bot added needs-ci PRs that need a full CI run. path Issues and PRs related to the path subsystem. labels May 9, 2021
@nodejs-github-bot
Copy link
Collaborator

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

@targos
Copy link
Member

targos commented May 10, 2021

Technically, it changes the behavior for edge cases like path.toNamespacedPath([]).

@VoltrexKeyva
Copy link
Member Author

Technically, it changes the behavior for edge cases like path.toNamespacedPath([]).

Passing an empty array ([]) would still return the array itself since we first check if the type of the value isn't a string -> (if type is not string or length of the value is 0, return value).
I don't see a behavior change here though.

@targos
Copy link
Member

targos commented May 10, 2021

You're right, sorry about that 🤦‍♂️

@nodejs-github-bot
Copy link
Collaborator

nodejs-github-bot commented May 10, 2021 •
edited by jasnell
Loading

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

@RaisinTen RaisinTen added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label May 10, 2021
@jasnell jasnell added fast-track PRs that do not need to wait for 48 hours to land. and removed needs-ci PRs that need a full CI run. labels May 10, 2021
@github-actions
Copy link
Contributor

Fast-track has been requested by @jasnell. Please 👍 to approve.

@Trott

This comment has been minimized.

Sign in to view
@Trott Trott removed the fast-track PRs that do not need to wait for 48 hours to land. label May 12, 2021
@lpinca
Copy link
Member

lpinca commented May 12, 2021

@Trott what is the behavior change?

@Trott
Copy link
Member

Trott commented May 12, 2021

@Trott what is the behavior change?

@lpinca I didn't want to provide an example until I compiled with the change and tested. And now that I've done that...uh...yeah, the types of things I was thinking about don't change the behavior. Uh..whoops.

@Trott Trott added the fast-track PRs that do not need to wait for 48 hours to land. label May 12, 2021
@github-actions
Copy link
Contributor

Fast-track has been requested by @Trott. Please 👍 to approve.

@jasnell
Copy link
Member

jasnell commented May 12, 2021

Landed in 89f592c

@jasnell jasnell closed this May 12, 2021
jasnell pushed a commit that referenced this pull request May 12, 2021
@VoltrexKeyva @jasnell
path: inline conditions
89f592c 
This condition can be inlined in the first `if` statement since
if the `path`'s length is 0, it'll be a empty string so we can
return that as there's no need for an extra `if` statement.

PR-URL: #38613
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@VoltrexKeyva VoltrexKeyva deleted the patch-3 branch May 12, 2021 15:13
targos pushed a commit that referenced this pull request May 17, 2021
@VoltrexKeyva @targos
path: inline conditions
be4b3a4 
This condition can be inlined in the first `if` statement since
if the `path`'s length is 0, it'll be a empty string so we can
return that as there's no need for an extra `if` statement.

PR-URL: #38613
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@targos targos mentioned this pull request May 18, 2021
Merged
targos pushed a commit that referenced this pull request May 30, 2021
@VoltrexKeyva @targos
path: inline conditions
c738b73 
This condition can be inlined in the first `if` statement since
if the `path`'s length is 0, it'll be a empty string so we can
return that as there's no need for an extra `if` statement.

PR-URL: #38613
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
targos pushed a commit that referenced this pull request Jun 5, 2021
@VoltrexKeyva @targos
path: inline conditions
838a619 
This condition can be inlined in the first `if` statement since
if the `path`'s length is 0, it'll be a empty string so we can
return that as there's no need for an extra `if` statement.

PR-URL: #38613
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
targos pushed a commit that referenced this pull request Jun 5, 2021
@VoltrexKeyva @targos
path: inline conditions
16509d3 
This condition can be inlined in the first `if` statement since
if the `path`'s length is 0, it'll be a empty string so we can
return that as there's no need for an extra `if` statement.

PR-URL: #38613
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
@targos targos mentioned this pull request Jun 6, 2021
Merged
targos pushed a commit that referenced this pull request Jun 11, 2021
@VoltrexKeyva @targos
path: inline conditions
daeb6fc 
This condition can be inlined in the first `if` statement since
if the `path`'s length is 0, it'll be a empty string so we can
return that as there's no need for an extra `if` statement.

PR-URL: #38613
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Richard Lau <[email protected]>
Reviewed-By: Darshan Sen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RaisinTen RaisinTen RaisinTen approved these changes

@marsonya marsonya marsonya approved these changes

@richardlau richardlau richardlau approved these changes

@cjihrig cjihrig cjihrig approved these changes

@targos targos targos approved these changes

@lpinca lpinca lpinca approved these changes

@Trott Trott Trott approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. fast-track PRs that do not need to wait for 48 hours to land. path Issues and PRs related to the path subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@VoltrexKeyva @nodejs-github-bot @targos @Trott @lpinca @jasnell @cjihrig @richardlau @marsonya @RaisinTen