FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Remove unused branches from fs by benjamingr · Pull Request #4795 · nodejs/node · GitHub

/ node Public

Remove unused branches from fs - #4795

Closed
benjamingr wants to merge 1 commit into
nodejs:masterfrom
benjamingr:patch-2
ConversationCommits (1)ChecksFiles changed
Closed

Remove unused branches from fs#4795
benjamingr wants to merge 1 commit into
nodejs:masterfrom
benjamingr:patch-2

Conversation

benjamingr commented Jan 21, 2016

Copy link
Copy Markdown
Member

In a few places the code was refactored to use maybeCallback which always returns a function. Checking for if (callback) always returns true anyway.

mscdex added the fs Issues and PRs related to the fs subsystem / file system. label Jan 21, 2016

cjihrig commented Jan 21, 2016

Copy link
Copy Markdown
Contributor

LGTM if the CI is happy

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

thefourtheye commented Jan 21, 2016

Copy link
Copy Markdown
Contributor

LGTM

benjamingr commented Jan 21, 2016

Copy link
Copy Markdown
Member Author

Any idea why the CI isn't happy? Doesn't seem related.

cjihrig commented Jan 21, 2016

Copy link
Copy Markdown
Contributor

Just Jenkins being Jenkins. I just got a green build though, so trying again: https://ci.nodejs.org/job/node-test-pull-request/1347/

jasnell commented Jan 21, 2016

Copy link
Copy Markdown
Member

LGTM

fs: Remove unused branches
1aa592f 
In a few places the code was refactored to use `maybeCallback` which
always returns a function. Checking for `if (callback)` always returns true anyway.

PR-URL: nodejs#4795
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: thefourtheye <thechargingvolcano@gmail.com>

benjamingr commented Jan 22, 2016

Copy link
Copy Markdown
Member Author

Ok, fixed commit message as far as I'm concerned this can land and it got 3 LGTMs. Thanks for the fast feedback as always :)

jasnell commented Jan 22, 2016

Copy link
Copy Markdown
Member

Weird build bot failures happening in CI right now... just to be safe following the new commit, new CI run: https://ci.nodejs.org/job/node-test-pull-request/1351/

benjamingr commented Jan 22, 2016

Copy link
Copy Markdown
Member Author

@jasnell good idea. Passed.

silverwind pushed a commit that referenced this pull request Jan 23, 2016
fs: remove unused branches
c00d08f 
In a few places the code was refactored to use `maybeCallback` which
always returns a function. Checking for `if (callback)` always returns
true anyway.

PR-URL: #4795
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: thefourtheye <thechargingvolcano@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>

silverwind commented Jan 23, 2016

Copy link
Copy Markdown
Contributor

Thanks! Landed in c00d08f.

@benjamingr Thanks for pre-filling the review lines! I lowercased the commit title and wrapped the message body at 72 character, for reference. 😉

silverwind closed this Jan 23, 2016

benjamingr commented Jan 24, 2016

Copy link
Copy Markdown
Member Author

@silverwind thanks, I totally forgot about that - been a while since I've been able to contribute a PR :)

rvagg pushed a commit that referenced this pull request Jan 25, 2016
fs: remove unused branches
fa940cf 
In a few places the code was refactored to use `maybeCallback` which
always returns a function. Checking for `if (callback)` always returns
true anyway.

PR-URL: #4795
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: thefourtheye <thechargingvolcano@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
rvagg mentioned this pull request Feb 8, 2016
Closed

MylesBorins commented Feb 17, 2016

Copy link
Copy Markdown
Contributor

@benjamingr it looks like this PR is modifying a bunch of code paths that don't exist on v4.x-staging and thus we are getting a ton of conflicts.

Would you be willing to manually backport?

benjamingr commented Feb 17, 2016

Copy link
Copy Markdown
Member Author

@thealphanerd sure, do I just open a pull request against the v4 branch?

MylesBorins commented Feb 17, 2016

Copy link
Copy Markdown
Contributor

against v4.x-staging

benjamingr mentioned this pull request Feb 17, 2016
Closed
scovetta pushed a commit to scovetta/node that referenced this pull request Apr 2, 2016
fs: remove unused branches
682e21d 
In a few places the code was refactored to use `maybeCallback` which
always returns a function. Checking for `if (callback)` always returns
true anyway.

PR-URL: nodejs#4795
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: James M Snell <jasnell@gmail.com>
Reviewed-By: thefourtheye <thechargingvolcano@gmail.com>
Reviewed-By: Roman Reiss <me@silverwind.io>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

fs Issues and PRs related to the fs subsystem / file system.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

Back | FazBrowse Home | New Git URL