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

stream: name anonymous function in _stream_writable.js #21753

Closed
wants to merge 1 commit into from
Closed

stream: name anonymous function in _stream_writable.js #21753

wants to merge 1 commit into from

Conversation

mariotsi
Copy link
Contributor

Refs: #8913

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

@nodejs-github-bot nodejs-github-bot added the stream Issues and PRs related to the stream subsystem. label Jul 11, 2018
@trivikr
Copy link
Member

trivikr commented Jul 11, 2018

Thank you @mariotsi for raising your first PR in Node.js core! 🎉

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

lpinca
lpinca approved these changes Jul 12, 2018
View reviewed changes
lib/_stream_writable.js Outdated
@@ -190,7 +190,7 @@ if (typeof Symbol === 'function' && Symbol.hasInstance) {
}
});
} else {
realHasInstance = function(object) {
realHasInstance = function realHasInstance(object) {
Copy link
Member

@lpinca lpinca Jul 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

This is not needed, the name is inferred from the variable name.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, removed

@mariotsi mariotsi force-pushed the anonymous-functions branch 2 times, most recently from 6022077 to 99f0f13 Compare July 12, 2018 09:58
@mariotsi mariotsi force-pushed the anonymous-functions branch from 99f0f13 to 6e836af Compare July 12, 2018 10:01
@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 12, 2018
@mariotsi
Copy link
Contributor Author

@BridgeAR just checking the meaning of the "author ready" and I'm wondering what LG stands for

@trivikr
Copy link
Member

trivikr commented Jul 13, 2018

author-ready label is used for PRs that can land (details)
LG is "Looks good" I think (reviewing wiki)

maclover7
maclover7 approved these changes Jul 13, 2018
View reviewed changes
Copy link
Contributor

@maclover7 maclover7 left a comment

Choose a reason for hiding this comment

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

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

@mariotsi
Copy link
Contributor Author

@maclover7 one task if the CI is running from two days.

@Trott
Copy link
Member

Trott commented Jul 16, 2018

Linter only needs a re-run. I'm unable to restart it right now for some reason.

@trivikr
Copy link
Member

trivikr commented Jul 18, 2018

node-test-linter re-run https://ci.nodejs.org/job/node-test-linter/20693/

@BridgeAR
Copy link
Member

BridgeAR commented Jul 18, 2018 •
edited
Loading

Linter CI https://ci.nodejs.org/job/node-test-linter/20702/ ✔️

[edit] I should refresh next time ;-) [/edit]

@mariotsi
Copy link
Contributor Author

I still see node-test-commit running from 3 days

BridgeAR pushed a commit to BridgeAR/node that referenced this pull request Jul 18, 2018
@mariotsi @BridgeAR
stream: name anonymous function in _stream_writable.js
caf2335 
PR-URL: nodejs#21753
Refs: nodejs#8913
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@BridgeAR
Copy link
Member

Landed in caf2335 🎉

@mariotsi congratulations on your first commit to Node.js! :-) The CI did not detect the rerun of the linter. But all checks passed.

@BridgeAR BridgeAR closed this Jul 18, 2018
targos pushed a commit that referenced this pull request Jul 19, 2018
@mariotsi @targos
stream: name anonymous function in _stream_writable.js
6af4f1f 
PR-URL: #21753
Refs: #8913
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Jon Moss <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
@mariotsi mariotsi deleted the anonymous-functions branch July 19, 2018 09:34
@mariotsi
Copy link
Contributor Author

Thanks :) See you on the next PR!

@targos targos removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jul 19, 2018
@targos targos mentioned this pull request Jul 31, 2018
Merged
@beevelop beevelop mentioned this pull request Aug 1, 2018
Closed
This was referenced Aug 1, 2018
Closed
Closed
Closed
Closed
Closed
@snyk-bot snyk-bot mentioned this pull request Apr 1, 2020
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@trivikr trivikr trivikr approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

@maclover7 maclover7 maclover7 approved these changes

@cjihrig cjihrig cjihrig approved these changes

@lpinca lpinca lpinca approved these changes

@addaleax addaleax addaleax approved these changes

@jasnell jasnell jasnell approved these changes

Assignees
No one assigned
Labels
stream Issues and PRs related to the stream subsystem.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.