http: remove pointless uses of arguments#10664
Uh oh!
There was an error while loading. Please reload this page.
Conversation
nodejs-github-bot added fs 
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 commented Jan 6, 2017
I was trying to avoid semver-major, but I can make that change. If |
targos commented Jan 6, 2017
+1 for the early error but the message should be updated to say that a string is required |
lib/_http_outgoing.js Outdated
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Good point... hmmm... yeah, there may not be a way around making this a semver-major
jasnell commented Jan 6, 2017
Changing the message to indicate that a string is required would force it to be semver-major |
cjihrig commented Jan 6, 2017
I'm going to update the error and make it an uncontroversial semver major. |
cjihrig added the semver-major 
lib/_http_outgoing.js Outdated
jasnell commented Jan 6, 2017
A new/updated test may be worthwhile also |
cjihrig changed the title cjihrig commented Jan 10, 2017
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]>
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]>
PR-URL: #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]>
PR-URL: nodejs#10664 Reviewed-By: James M Snell <[email protected]> Reviewed-By: Michaël Zasso <[email protected]> Reviewed-By: Luigi Pinca <[email protected]>
6 participants
Checklist
make -j4 test(UNIX), orvcbuild test(Windows) passesAffected core subsystem(s)
lib
Use of
argumentsgenerally only causes problems. This PR removes several uses that serve no purpose at all.