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

tools: auto fix custom eslint rule for buffer-constructor.js #16664

Conversation

shobhitchittora
Copy link
Contributor

@shobhitchittora shobhitchittora commented Nov 1, 2017

This implements an es-lint fixer method to fix the usage of Deprecated Buffer() constructor.

Also adds some more test cases to the eslint rule.

Refs: #16636

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

Tools

This implements a fixer method to fix the usage of Deprecated Buffer() constructor.

Also adds some more test cases to the eslint rule.

Refs: nodejs#16636
@nodejs-github-bot nodejs-github-bot added the tools Issues and PRs related to the tools directory. label Nov 1, 2017
This extends the current test for the custom eslint rule with expected output key.

Refs: nodejs#16636
@starkwang
Copy link
Contributor

The new Buffer(size) is not equivalent to Buffer.from(...):
https://nodejs.org/dist/latest-v8.x/docs/api/buffer.html#buffer_buffer_from_buffer_alloc_and_buffer_allocunsafe

@shobhitchittora
Copy link
Contributor Author

@starkwang So if I understand correctly, if the argument to Buffer constructor is number, then Buffer.alloc(size) should be used. Otherwise Buffer.from() can be used.

PS: Thanks for pointing it out.

@jasnell
Copy link
Member

jasnell commented Nov 2, 2017

I'm a bit less keen on having an auto fix for this one, at least not yet. Which of the new constructors is used depends entirely on the type of input, which is not always clear. It's also important to consider between Buffer.alloc() and Buffer.allocUnsafe().

@apapirovski
Copy link
Member

I don't think we need an auto-fixer for every lint rule. This one is a bit ambiguous so I tend to agree with @jasnell

@shobhitchittora
Copy link
Contributor Author

@jasnell +1 that. Should I close this PR then?

@jasnell
Copy link
Member

jasnell commented Nov 3, 2017

Yeah, let's close this one... just please don't let that discourage you! This work is very appreciated!

@jasnell jasnell closed this Nov 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
tools Issues and PRs related to the tools directory.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants