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: validate arg for compare #23878

Closed
wants to merge 5 commits into from
Closed

buffer: validate arg for compare #23878

wants to merge 5 commits into from

Conversation

refack
Copy link
Contributor

@refack refack commented Oct 25, 2018

Refs: #23840

Makes server-major changes to buf.compare() argument validation, to be in line with docs (require areg to be Number. Disable coerce string like 0xff);

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

@nodejs-github-bot
Copy link
Collaborator

@refack build started: https://ci.nodejs.org/blue/organizations/jenkins/node-test-pull-request-lite-pipeline/detail/node-test-pull-request-lite-pipeline/1367/pipeline

@nodejs-github-bot nodejs-github-bot added the buffer Issues and PRs related to the buffer subsystem. label Oct 25, 2018
@refack refack added the semver-major PRs that contain breaking changes and should be released in the next major version. label Oct 25, 2018
@refack refack mentioned this pull request Oct 25, 2018
Closed
3 tasks
@Trott Trott force-pushed the validate-buffer-params-compare branch from b48426d to ab4486b Compare November 30, 2018 04:26
@Trott
Copy link
Member

Trott commented Nov 30, 2018

@nodejs/buffer

@Trott
Copy link
Member

Trott commented Dec 4, 2018

Semver major, so would need one more approval from @nodejs/tsc.

mcollina
mcollina approved these changes Dec 5, 2018
View reviewed changes
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

@Trott
Copy link
Member

Trott commented Dec 6, 2018

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

@Trott Trott added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 6, 2018
@Trott
Copy link
Member

Trott commented Dec 6, 2018

Seems to break many many tests which I guess is due to changes that happened since this was opened? Anyway, will put this one on hold for now.

Trott
Trott requested changes Dec 6, 2018
View reviewed changes
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

Seems to break many many tests which I guess is due to changes that happened since this was opened? Anyway, will put this one on hold for now.

@trivikr trivikr removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Dec 8, 2018
@refack refack closed this Dec 21, 2018
@refack refack deleted the validate-buffer-params-compare branch December 21, 2018 01:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Trott Trott Trott requested changes

@jasnell jasnell jasnell approved these changes

@mcollina mcollina mcollina approved these changes

Assignees
No one assigned
Labels
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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@refack @nodejs-github-bot @Trott @mcollina @jasnell @trivikr