Skip to content

buffer: allow invalid encoding in from - #54533

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
nxtedition:invalid-encodinf-utf8
Aug 27, 2024
Merged

buffer: allow invalid encoding in from#54533
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
nxtedition:invalid-encodinf-utf8

Conversation

@ronag

@ronag ronag commented Aug 24, 2024

Copy link
Copy Markdown
Member

Looks like a bug to me but the change should probably done in a semver majpr.

@nodejs-github-bot nodejs-github-bot added buffer Issues and PRs related to the buffer subsystem. needs-ci PRs that need a full CI run. labels Aug 24, 2024
mcollina
mcollina approved these changes Aug 24, 2024
View reviewed changes

@mcollina mcollina left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm,

+1 on changing it in a semver-major.

@ronag
ronag requested a review from RafaelGSS August 24, 2024 07:53
@codecov

ghost commented Aug 24, 2024 •
edited by codecov Bot
Loading

Copy link
Copy Markdown

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.33%. Comparing base (9ee3a72) to head (e25c344).
Report is 26 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #54533      +/-   ##
==========================================
- Coverage   87.33%   87.33%   -0.01%     
==========================================
  Files         649      649              
  Lines      182561   182603      +42     
  Branches    35041    35034       -7     
==========================================
+ Hits       159444   159480      +36     
- Misses      16378    16401      +23     
+ Partials     6739     6722      -17
Files Coverage Δ
lib/buffer.js 96.70% <100.00%> (ø)

... and 28 files with indirect coverage changes

@ronag ronag added author ready PRs with CI started, the required approvals, and no outstanding review comments. request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. labels Aug 24, 2024
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Aug 24, 2024
@nodejs-github-bot

ghost commented Aug 24, 2024

Copy link
Copy Markdown
Collaborator

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

@avivkeller avivkeller added the semver-major PRs that contain breaking changes and should be released in the next major version. label Aug 24, 2024
@avivkeller

ghost commented Aug 24, 2024

Copy link
Copy Markdown
Member

Given the description and @mcollina's comment, I've added the semver-major label. Feel free to adjust.

@targos targos removed the semver-major PRs that contain breaking changes and should be released in the next major version. label Aug 24, 2024
@targos

ghost commented Aug 24, 2024

Copy link
Copy Markdown
Member

It's not this change that should be done in a major. It's the one that this is reverting.

Robert Nagy
buffer: allow invalid encoding in from
e25c344 
Looks like a bug to me but the change should probably
done in a semver majpr.

PR-URL: nodejs#54533
@ronag ronag added the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Aug 24, 2024
@ronag
ronag force-pushed the invalid-encodinf-utf8 branch from d44ff2a to e25c344 Compare August 24, 2024 14:22
@github-actions github-actions Bot added request-ci-failed Starting CI with the request-ci label failed and requires manual intervention. and removed request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. labels Aug 24, 2024
@github-actions

ghost commented Aug 24, 2024

Copy link
Copy Markdown
Contributor
Failed to start CI
   ⚠  Something was pushed to the Pull Request branch since the last approving review.
   ✘  Refusing to run CI on potentially unsafe PR
https://github.com/nodejs/node/actions/runs/10539235594

@ronag
ronag requested review from benjamingr and mcollina August 26, 2024 05:07
@ronag ronag mentioned this pull request Aug 26, 2024
Merged
mertcanaltin
mertcanaltin approved these changes Aug 26, 2024
View reviewed changes

ghost left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm

@jakecastelli jakecastelli added request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. and removed request-ci-failed Starting CI with the request-ci label failed and requires manual intervention. labels Aug 26, 2024
@github-actions github-actions Bot removed the request-ci Add this label to start a Jenkins CI on a PR. Only starts once the PR has an approving review. label Aug 26, 2024
@nodejs-github-bot

ghost commented Aug 26, 2024

Copy link
Copy Markdown
Collaborator

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

@nodejs-github-bot

ghost commented Aug 26, 2024

Copy link
Copy Markdown
Collaborator

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

@nodejs-github-bot

ghost commented Aug 26, 2024

Copy link
Copy Markdown
Collaborator

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

@ronag ronag added the commit-queue PRs queued for automated landing through the Commit Queue. label Aug 27, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue PRs queued for automated landing through the Commit Queue. label Aug 27, 2024
@nodejs-github-bot
nodejs-github-bot merged commit c00ea01 into nodejs:main Aug 27, 2024
@nodejs-github-bot

ghost commented Aug 27, 2024

Copy link
Copy Markdown
Collaborator

Landed in c00ea01

@ronag ronag mentioned this pull request Aug 27, 2024
Closed
@targos targos mentioned this pull request Sep 30, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@RafaelGSS RafaelGSS RafaelGSS approved these changes
@jakecastelli jakecastelli jakecastelli approved these changes
@targos targos Awaiting requested review from targos
@mcollina mcollina Awaiting requested review from mcollina
@benjamingr benjamingr Awaiting requested review from benjamingr
+1 more reviewer
@mertcanaltin mertcanaltin mertcanaltin approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author ready PRs with CI started, the required approvals, and no outstanding review comments. buffer Issues and PRs related to the buffer subsystem. needs-ci PRs that need a full CI run.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@ronag @nodejs-github-bot @avivkeller @targos @mcollina @benjamingr @RafaelGSS @mertcanaltin @jakecastelli