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

buffer: do not always use defaults #20054

Closed
wants to merge 1 commit into from

Conversation

BridgeAR
Copy link
Member

The Buffer#(read|write)U?Int(B|L)E functions should not use a default
value. This is very likely a bug and it was never documented that
way.

I checked this against gzemnid and it did not show a single module using
this pattern. I personally say it is a bug if the user calls these functions
without arguments, because it is not clear what happens.

Besides that this also improves the tests by adding more tests and by
refactoring them to less code lines.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

The Buffer#(read|write)U?Int(B|L)E functions should not use a default
value. This is very likely a bug and it was never documented that
way.

Besides that this also improves the tests by adding more tests and by
refactoring them to less code lines.
@BridgeAR BridgeAR added the semver-major PRs that contain breaking changes and should be released in the next major version. label Apr 15, 2018
@BridgeAR BridgeAR requested review from ChALkeR and addaleax April 15, 2018 20:12
@nodejs-github-bot nodejs-github-bot added the buffer Issues and PRs related to the buffer subsystem. label Apr 15, 2018
@BridgeAR BridgeAR requested a review from a team April 15, 2018 20:12
@BridgeAR
Copy link
Member Author

Copy link
Member

@addaleax addaleax 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 requested a review from a team April 15, 2018 21:38
@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Apr 15, 2018
@rvagg
Copy link
Member

rvagg commented Apr 16, 2018

LGTM I suppose, seems a bit late to be squeezing something like this into 10 though

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

Copy link
Member

@ChALkeR ChALkeR left a comment

Choose a reason for hiding this comment

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

LGTM.

Nit though: part of test style changes look unrelated, and I am not sure if those should be done in a semver-major commit. /cc @MylesBorins

@BridgeAR
Copy link
Member Author

@ChALkeR I can split the tests into a different PR if you want. But since it is work: are you fine with landing it as is?

BridgeAR added a commit to BridgeAR/node that referenced this pull request Apr 29, 2018
The Buffer#(read|write)U?Int(B|L)E functions should not use a default
value. This is very likely a bug and it was never documented that
way.

Besides that this also improves the tests by adding more tests and by
refactoring them to less code lines.

PR-URL: nodejs#20054
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Khaidi Chu <[email protected]>
Reviewed-By: Matteo Collina <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Trivikram Kamat <[email protected]>
Reviewed-By: Сковорода Никита Андреевич <[email protected]>
@BridgeAR
Copy link
Member Author

No one complaint in the last 11 days, so I went ahead and landed it as is.

Landed in 60b5b38

@BridgeAR BridgeAR closed this Apr 29, 2018
targos added a commit to targos/node that referenced this pull request Jun 6, 2018
targos added a commit that referenced this pull request Jun 8, 2018
Backport of a changed test from
#20054

PR-URL: #21170
Reviewed-By: Colin Ihrig <[email protected]>
targos added a commit that referenced this pull request Jun 13, 2018
Backport of a changed test from
#20054

PR-URL: #21170
Reviewed-By: Colin Ihrig <[email protected]>
@BridgeAR BridgeAR deleted the stricter-buffer-defaults branch April 1, 2019 23:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. buffer Issues and PRs related to the buffer subsystem. semver-major PRs that contain breaking changes and should be released in the next major version.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

9 participants