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

http: remove pointless uses of arguments #10664

Merged
merged 2 commits into from
Jan 10, 2017
Merged

Conversation

cjihrig
Copy link
Contributor

@cjihrig cjihrig commented Jan 6, 2017

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)

lib

Use of arguments generally only causes problems. This PR removes several uses that serve no purpose at all.

@nodejs-github-bot nodejs-github-bot added fs Issues and PRs related to the fs subsystem / file system. http Issues or PRs related to the http subsystem. labels Jan 6, 2017
@mscdex
Copy link
Contributor

mscdex commented Jan 6, 2017

With the http changes for example, the error messages now no longer reflect what is being tested, so they would need to be changed. Either way those http changes might be semver-major because of the change in behavior?

@cjihrig
Copy link
Contributor Author

cjihrig commented Jan 6, 2017

I was trying to avoid semver-major, but I can make that change. If name isn't a string, toLowerCase() will crash later on in both functions.

@targos
Copy link
Member

targos commented Jan 6, 2017

+1 for the early error but the message should be updated to say that a string is required

@@ -382,7 +382,7 @@ OutgoingMessage.prototype.setHeader = function setHeader(name, value) {


OutgoingMessage.prototype.getHeader = function getHeader(name) {
if (arguments.length < 1) {
if (typeof name !== 'string') {
Copy link
Member

Choose a reason for hiding this comment

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

hmm... I think this technically makes this a semver-major because of the change in why the error is thrown, although there would be no way of triggering that difference so it likely wouldn't matter. Therefore I'm +1 with this part as semver-patch

Copy link
Contributor

Choose a reason for hiding this comment

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

What do you mean? If someone does getHeader(undefined) for example it now throws a different error. An argument was supplied, now it's just the wrong type.

Copy link
Member

Choose a reason for hiding this comment

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

Good point... hmmm... yeah, there may not be a way around making this a semver-major

@jasnell
Copy link
Member

jasnell commented Jan 6, 2017

Changing the message to indicate that a string is required would force it to be semver-major

@cjihrig
Copy link
Contributor Author

cjihrig commented Jan 6, 2017

I'm going to update the error and make it an uncontroversial semver major.

@cjihrig cjihrig added the semver-major PRs that contain breaking changes and should be released in the next major version. label Jan 6, 2017
if (arguments.length < 1) {
throw new Error('"name" argument is required for getHeader(name)');
if (typeof name !== 'string') {
throw new Error('"name" argument must be a string');
Copy link
Member

Choose a reason for hiding this comment

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

TypeError

@jasnell
Copy link
Member

jasnell commented Jan 6, 2017

A new/updated test may be worthwhile also

@cjihrig cjihrig changed the title lib: remove pointless uses of arguments http: remove pointless uses of arguments Jan 10, 2017
@cjihrig
Copy link
Contributor Author

cjihrig commented Jan 10, 2017

The changes to lib/fs.js are gone since 3c2a936 landed.

Pushed another commit that updates the relevant test @jasnell.

@cjihrig
Copy link
Contributor Author

cjihrig commented Jan 10, 2017

PR-URL: nodejs#10664
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
PR-URL: nodejs#10664
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@cjihrig cjihrig merged commit 2d2a059 into nodejs:master Jan 10, 2017
@cjihrig cjihrig deleted the arguments branch January 10, 2017 19:05
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 18, 2017
PR-URL: nodejs#10664
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 18, 2017
PR-URL: nodejs#10664
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
targos pushed a commit that referenced this pull request Jan 28, 2017
PR-URL: #10664
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 30, 2017
PR-URL: nodejs#10664
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
italoacasas pushed a commit to italoacasas/node that referenced this pull request Jan 30, 2017
PR-URL: nodejs#10664
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Michaël Zasso <[email protected]>
Reviewed-By: Luigi Pinca <[email protected]>
@italoacasas italoacasas mentioned this pull request Jan 31, 2017
@jasnell jasnell mentioned this pull request Apr 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
fs Issues and PRs related to the fs subsystem / file system. http Issues or PRs related to the http 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.

6 participants