Throw on code 306 #19
Conversation
|
@jonchurch makes sense 👍 Currently it is not throwing and we are able to log the code to stdout: Not sure if it aggregates as extra info but I found this thread on IANA codes nodejs/node#1470 To be clear: Then if the idea is to throw, according the docs: |
|
Yea, it looks like there are two issues here: (1) docs issue: the throw is unrelated to if a code is "supported" by Node.js; it is just if a code is known to exist |
|
Updated the PR to filter Note: IANA seems to require a I updated the fetch script to include I also added a regression test to make sure code 306 throws an error. Finally, I updated the documentation to indicate 306 is no longer supported by the HTTP spec |
|
Ah, I was really curious why the User-Agent was added, thanks for the explaination 👍 |
scripts/fetch-iana.js
Outdated
| @@ -19,7 +20,7 @@ https.get(URL, function onResponse (res) { | |||
| rows.forEach(function (row) { | |||
| var obj = row.reduce(reduceRows, {}) | |||
|
|
|||
| if (obj.description !== 'Unassigned') { | |||
| if (obj.description !== 'Unassigned' && obj.value !== '306') { | |||
There was a problem hiding this comment.
I'm kind of on the fence on if this should specifically blacklist 306 code, or maybe just ones where the message is (unused) or similar. I was originally picturing in my mind the blacklist being against (Unused), but of course there is no other code like it, so it wouldn't really make a difference. Since a reason phrase can be any obs-text, there is nothing wrong with it per-se, so I suppose a blacklist on the specific code is good enough.
There was a problem hiding this comment.
I'm on the fence too, but I think it serves as self documentation. It's a unique case, so handling it in code as unique seems appropriate.
The logical question would be, do we want to remove any code that in the future is documented as (Unused)?
I can change it right now if you'd prefer to see us filter out all (Unused), otherwise I think singling out 306 is appropriate.
There was a problem hiding this comment.
No, sorry, it was just a comment :) not a request for a change. I just after that followed the IANA registration and I think there is no clear (good) way to make such a distinction, so a filter on the code seems appropriate.
There was a problem hiding this comment.
Since this does not need to be performant or anything and there are two filters, I flipped them so we could add a comment about why 306 is ignore for reference :)
|
Hye @jonchurch I'm going to cherry-pick your user-agent change on to master prior to merging this PR; this is mainly because it's logically a separate change from this, though you happened to find it while working on this (great find!). I hope that is OK. |
dougwilson
force-pushed
the
remove-code-306
branch
from
72f75c0 to
1ca6960
Compare
|
No problem |
|
Cool. LMK if you have any issue with a couple nit fixes I pushed up here 👍 . I'm working to get this module release out as a 2.0 finally and bring this repo down to 0 issues / 0 PRs. |
|
LGTM |
|
Nice. Thanks for your work on this @jonchurch ; as we continue to push and clean up these repos, we will get closer and closer to a place where PRs are more likely to land quickly 🎉 |
dougwilson
force-pushed
the
remove-code-306
branch
from
668f0c7 to
eab4c63
Compare
|
Changes squashed and landed. |
This is just a suggestion based on issue #18.
I have removed code 306 after
codes.jsonis required to makestatus(306)throw as is indicated in the docs.I assume this would be a breaking change.
The main question is:
Should 306 throw an error?
Assuming it should, when fetching the IANA codes we could remove it from the response before writing it to
iana.json. Or it could be manually edited like I see has been done for some other codes like in c46c4d4.If 306 should not throw, the docs can just be updated.