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

Update status codes to official IANA list #100

Merged
merged 3 commits into from Mar 27, 2018

Conversation

vanHoesel
Copy link
Member

@vanHoesel vanHoesel commented Mar 12, 2018

This updates the Status Codes to the official IANA Registry as described at:

https://www.iana.org/assignments/http-status-codes/http-status-codes.xhtml

I did remove a few that were not part of the official list, like 418: I'm a teapot. But then again include some existing unofficial ones that were already in the old version in a separate module.

Maybe, in the future we can use an import like:

use HTTP::Status ':nginx'

to include the 444, 495, 496, 497, and 499, for example.

@vanHoesel
Copy link
Member Author

this would probably make it possible to close #17 (Add HTTP status codes 420, 421, 451, 508), considering the comments on that one

200 => 'OK',
201 => 'Created',
201 => 'Created',
Copy link
Member

Choose a reason for hiding this comment

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

this looks unintentional

208 => 'Already Reported', # RFC 5842
206 => 'Partial Content', # RFC 7233: Range Requests
207 => 'Multi-Status', # RFC 4918: WebDAV
208 => 'Already Reported', # RFC 5842: WebDAV bindings
Copy link
Member

Choose a reason for hiding this comment

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

odd whitespace

429 => 'Too Many Requests',
431 => 'Request Header Fields Too Large',
449 => 'Retry with', # unofficial Microsoft
# 418 .. 420
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we should remove any pre-existing entries: someone might be using them.

Copy link
Member Author

Choose a reason for hiding this comment

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

they were 'temporary' removed, to make the list IANA compliant ... intentionally
but I also planned to re add them.

I would not intentionally break people their stuff, quite aware that there is code out there in the wild.

But thanks for pointing out


=cut

use DDP; p %StatusCode;
Copy link
Member

Choose a reason for hiding this comment

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

debugging code left behind?

@@ -91,6 +91,13 @@ my %StatusCode = (
511 => 'Network Authentication Required', # RFC 6585: Additional Codes
);

require HTTP::Status::Unofficial;
Copy link
Member

Choose a reason for hiding this comment

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

I'm not convinced that this is a good idea. It's easier to just leave the codes where they are in the main list.

Copy link
Member Author

Choose a reason for hiding this comment

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

The idea I was toying with, was to make separate groups of codes in split files and that could be dynamically loaded.

Any new (proposed) RFC could than have it's own file, with it's own small documentation etc.

But ... lets leave that idea for now... me being ambitious

@@ -46,6 +46,4 @@ $StatusCode{'509'} = 'Bandwidth Limit Exceeded';

=cut

use DDP; p %StatusCode;
Copy link
Member

Choose a reason for hiding this comment

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

this commit should have been squashed with the original that added this line, so we never saw it. Splitting it out into a separate change adds no value.

Copy link
Member Author

Choose a reason for hiding this comment

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

:-( sorry, it was already done

@karenetheridge
Copy link
Member

Grouping specific codes together makes more sense to do with an export tag, rather than with a separate namespace like ::Unofficial. Regardless, I think we should look at that in a separate PR rather than in this one, which should stick to just adding more codes from the RFCs.

@vanHoesel
Copy link
Member Author

@karenetheridge , I have removed the 'unofficial' file and pulled the codes back into the main file again. But still in a separate section, rather then cluttering the list that now reflects 'official IANA'

Putting 'others' into the list of codes is something we can indeed keep for another discussion. Maybe I was a bit to eager here :-)

Copy link
Member

@skaji skaji left a comment

Choose a reason for hiding this comment

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

LGTM

Especially, thanks for pointing out the link
https://www.iana.org/assignments/http-status-codes/http-status-codes.xhtml.

@karenetheridge
Copy link
Member

Ok, onward! Can you squash your changes down to the final set of commits? (i.e. "if it was all perfect right from the start, this is what the commit(s) would look like").

@vanHoesel
Copy link
Member Author

@karenetheridge , on request, I've rewritten history ... as if all was perfect ... and with the small commits as if I knew what I was doing :-D

@oalders
Copy link
Member

oalders commented Mar 14, 2018

Did 425 get removed from the codes?

@vanHoesel
Copy link
Member Author

vanHoesel commented Mar 14, 2018

@oalders 👍 well spotted!

yes ... 425 has been removed, it is not an official with IANA registered code!

but ... bad me 😞 for not having it added to the list of unofficial numbers at the bottom.

I did check again a few times but it seems to be okay now.

Added also the "RC_NO_CODE" alias to "RC_UNORDERED_COLLECTION", which is the most common interpretation these days.

@karenetheridge
Copy link
Member

thanks Theo, it all looks good to me!

@oalders
Copy link
Member

oalders commented Mar 27, 2018

Thanks @vanHoesel, @skaji and @karenetheridge!

@oalders oalders merged commit 2d0c224 into libwww-perl:master Mar 27, 2018
@Grinnz
Copy link
Contributor

Grinnz commented Sep 2, 2020

FYI the backcompat added here only added aliases for RC_NO_CODE, not HTTP_NO_CODE which was already in use, the RC_* constants seem to be legacy and not documented otherwise...

@Grinnz
Copy link
Contributor

Grinnz commented Sep 2, 2020

PR #145 includes the missing backcompat alias

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants