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.

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

Consistency of method naming #30506

Closed
wants to merge 2 commits into from
Closed

Consistency of method naming #30506

wants to merge 2 commits into from

Conversation

meelash
Copy link

@meelash meelash commented Nov 16, 2019

referred to as readable.push several other times in transform documentation and also documented under readable, so makes sense to just stick with readable.push

Checklist
  • documentation is changed or added

referred to as readable.push several other times in transform documentation and also documented under readable, so makes sense to just stick with readable.push
@nodejs-github-bot nodejs-github-bot added doc Issues and PRs related to the documentations. stream Issues and PRs related to the stream subsystem. labels Nov 16, 2019
@mscdex
Copy link
Contributor

mscdex commented Nov 17, 2019

-1 I think it's more confusing this way as it looks more like a typo or copy and paste error.

@meelash
Copy link
Author

meelash commented Nov 17, 2019

-1 I think it's more confusing this way as it looks more like a typo or copy and paste error.

In that case, there are 3-4 places in that same section (implementing transform) where it is referred to as readable.push, so those should also be changed to transform.push...

@BridgeAR
Copy link
Member

@nodejs/streams @nodejs/documentation @ronag PTAL

@ronag
Copy link
Member

ronag commented Dec 25, 2019

I’m +-0. But I do agree it should be consistent one way or another.

@BridgeAR BridgeAR requested a review from mcollina January 2, 2020 15:13
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@BridgeAR
Copy link
Member

BridgeAR commented Jan 2, 2020

@mscdex do you still think this should not be changed? Or what would be your suggestion about having this consistent?

@mscdex
Copy link
Contributor

mscdex commented Jan 2, 2020

@BridgeAR I'm fine with either leaving it as-is since it should be known that Transform inherits from/implements Readable or changing readable.push to transform.push in the Transform documentation.

@BridgeAR
Copy link
Member

Ping @meelash

@meelash
Copy link
Author

meelash commented Jan 12, 2020

@BridgeAR present

@BridgeAR
Copy link
Member

Would you mind updating the doc as suggested by @mscdex?

@rexagod
Copy link
Member

rexagod commented Mar 15, 2020

Ping @meelash

Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

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

lgtm

@meelash
Copy link
Author

meelash commented Mar 15, 2020

Ping @meelash

apologies. This had slipped my mind. I have made the changes per the suggestion by @mscdex . Let me know if there's anything else I can do. Thanks.

Copy link
Member

@a0viedo a0viedo left a comment

Choose a reason for hiding this comment

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

LGTM

@BridgeAR BridgeAR force-pushed the master branch 2 times, most recently from 8ae28ff to 2935f72 Compare May 31, 2020 12:19
jasnell pushed a commit that referenced this pull request Jul 3, 2020
Consistency of method naming

referred to as readable.push several other times in transform
documentation and also documented under readable, so makes sense
to just stick with readable.push

PR-URL: #30506
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@jasnell
Copy link
Member

jasnell commented Jul 3, 2020

Landed in 1a9d631

@jasnell jasnell closed this Jul 3, 2020
MylesBorins pushed a commit that referenced this pull request Jul 14, 2020
Consistency of method naming

referred to as readable.push several other times in transform
documentation and also documented under readable, so makes sense
to just stick with readable.push

PR-URL: #30506
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Jul 14, 2020
MylesBorins pushed a commit that referenced this pull request Jul 16, 2020
Consistency of method naming

referred to as readable.push several other times in transform
documentation and also documented under readable, so makes sense
to just stick with readable.push

PR-URL: #30506
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
addaleax pushed a commit that referenced this pull request Sep 22, 2020
Consistency of method naming

referred to as readable.push several other times in transform
documentation and also documented under readable, so makes sense
to just stick with readable.push

PR-URL: #30506
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@codebytere codebytere mentioned this pull request Sep 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc Issues and PRs related to the documentations. stream Issues and PRs related to the stream subsystem.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants