Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Update HttpStatusCode enum#26727

Merged
caesar-chen merged 2 commits intodotnet:masterfrom
caesar-chen:statusCode
Feb 1, 2018
Merged

Update HttpStatusCode enum#26727
caesar-chen merged 2 commits intodotnet:masterfrom
caesar-chen:statusCode

Conversation

@caesar-chen
Copy link
Contributor

Closes: #4382

/cc: @dotnet/ncl

Copy link
Contributor

@davidsh davidsh left a comment

Choose a reason for hiding this comment

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

Left some comments.

Unused = 306,
UpgradeRequired = 426,
UseProxy = 305,
Processing = 102,
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add these new enum values in alphabetical ordering with the rest of them above.

Copy link
Member

Choose a reason for hiding this comment

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

Nit: I agree the new codes should be integrated with the rest, but would it not make more sense for them to be ordered by numeric value rather than by name?

Copy link
Contributor

@davidsh davidsh Feb 1, 2018

Choose a reason for hiding this comment

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

It turns out that in the ref *.CS file, they are ordered alphabetically. This was an artifact with the old tooling (before the code was moved to GitHub). Other ref *.CS files follow this same pattern.

In the implementation *.CS file, they are ordered numerically.

Copy link
Member

Choose a reason for hiding this comment

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

Ah, ok.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will fix.

UnsupportedMediaType = 415,
RequestedRangeNotSatisfiable = 416,
ExpectationFailed = 417,
// Removed status code: ImATeapot = 418.
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of saying "Removed status code"...please add a comment describing why we aren't "adding" this status code. Please add the various links from the discussion thread on #4382.

Copy link
Contributor

Choose a reason for hiding this comment

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

I.e. from the discussion thread...

It would be a mistake to add it to .NET now. See golang/go#21326, nodejs/node#14644, psf/requests#4238 and aspnet/HttpAbstractions#915

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok.

LoopDetected = 508,

NotExtended = 510,
NetworkAuthenticationRequired = 511,
Copy link
Contributor

Choose a reason for hiding this comment

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

You can remove the "," at the end of this line since it is the last enum value.

Copy link
Contributor

@rmkerr rmkerr left a comment

Choose a reason for hiding this comment

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

LGTM

@lindexi
Copy link
Member

lindexi commented May 4, 2020

picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
* Update HttpStatusCode enum

* address feedback


Commit migrated from dotnet/corefx@504781c
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants