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

buffer: increase coverage by removing dead code #15100

Merged
merged 1 commit into from
Sep 1, 2017
Merged

buffer: increase coverage by removing dead code #15100

merged 1 commit into from
Sep 1, 2017

Conversation

decareano
Copy link

@decareano decareano commented Aug 30, 2017 •
edited by refack
Loading

Buffer.js file. L196 if (value == null) guarantees obj != null so L406 is unnecessary.
Removing outer if statement L406 and L418.

Checklist

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

@jasnell
Copy link
Member

jasnell commented Aug 30, 2017

This seems to have a whole lot more in it that it needs to. Can I ask that you rework it a bit to limit it to just the one change?

@refack refack force-pushed the buffer_editing branch 2 times, most recently from 68a139d to 18967f4 Compare August 30, 2017 20:48
@refack
Copy link
Contributor

refack commented Aug 30, 2017 •
edited
Loading

At @decareano's request I cleaned it up.

Hello @decareano, and welcome. Thank you for the contribution 😉
If you haven't already, it's recommended you take a look at CONTRIBUTING.md guide (especially the part about "discuss and update"). Customarily PRs are kept open for at least 48 hour so that anyone interested gets a chance to comment or review.

@refack
Copy link
Contributor

refack commented Aug 30, 2017

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

@refack refack self-assigned this Aug 30, 2017
@mscdex mscdex added the buffer Issues and PRs related to the buffer subsystem. label Aug 30, 2017
@refack refack changed the title buffer: increase coverage buffer: increase coverage by removing dead code Aug 30, 2017
jasnell
jasnell approved these changes Sep 1, 2017
View reviewed changes
Copy link
Member

@jasnell jasnell left a comment

Choose a reason for hiding this comment

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

LGTM if CI is green

@decareano @refack
buffer: increase coverage by removing dead code
08984b2 
buffer.js:L196 `if (value == null)` guarantees `obj != null`
so L406+L418 are unnecessary.

PR-URL: nodejs#15100
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@refack refack merged commit 08984b2 into nodejs:master Sep 1, 2017
@refack
Copy link
Contributor

refack commented Sep 1, 2017

@decareano congratulations on landing your first code contribution. GitHub has promoted you from:
image
to:
image
🍾

@refack
Copy link
Contributor

refack commented Sep 2, 2017

tanks to @decareano buffer.js branch coverage went up from 99.66% (586/588)
image
to 99.83% (585/586), by reducing the number of branches 🎉
image

addaleax pushed a commit to addaleax/ayo that referenced this pull request Sep 5, 2017
@decareano @addaleax
buffer: increase coverage by removing dead code
e288983 
buffer.js:L196 `if (value == null)` guarantees `obj != null`
so L406+L418 are unnecessary.

PR-URL: nodejs/node#15100
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
MylesBorins pushed a commit that referenced this pull request Sep 10, 2017
@decareano @MylesBorins
buffer: increase coverage by removing dead code
616c3cf 
buffer.js:L196 `if (value == null)` guarantees `obj != null`
so L406+L418 are unnecessary.

PR-URL: #15100
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins MylesBorins mentioned this pull request Sep 10, 2017
Merged
MylesBorins pushed a commit that referenced this pull request Sep 12, 2017
@decareano @MylesBorins
buffer: increase coverage by removing dead code
b9e1f60 
buffer.js:L196 `if (value == null)` guarantees `obj != null`
so L406+L418 are unnecessary.

PR-URL: #15100
Reviewed-By: Refael Ackermann <[email protected]>
Reviewed-By: Ruben Bridgewater <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Colin Ihrig <[email protected]>
Reviewed-By: James M Snell <[email protected]>
@MylesBorins
Copy link
Contributor

Should this be backported to v6.x-staging? If yes please follow the guide and raise a backport PR, if no let me know or add the dont-land-on label.

@refack refack removed their assignment Oct 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@refack refack refack approved these changes

@jasnell jasnell jasnell approved these changes

@addaleax addaleax addaleax approved these changes

@lpinca lpinca lpinca approved these changes

@cjihrig cjihrig cjihrig approved these changes

@BridgeAR BridgeAR BridgeAR approved these changes

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

Successfully merging this pull request may close these issues.
9 participants
@decareano @jasnell @refack @MylesBorins @addaleax @lpinca @cjihrig @BridgeAR @mscdex