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

Make StatusCode allow 600-999 #438

Closed
wants to merge 4 commits into from
Closed

Conversation

quininer
Copy link
Contributor

Also change StatusCode to NonZeroU16, I think this will help performance.

see https://rust.godbolt.org/z/1Evhbx

src/status.rs Show resolved Hide resolved
@Matthias247
Copy link

You should describe the use-case for this. The addition of status codes with printable representations will increase the binary (rough estimate: 400 status codes * (3 bytes len + 8 bytes length indicator + align = 16bytes) = 6kB).
So this shouldn't be done unless there is also a reason for it.

I guess one could be proxying, where an upstream return an arbitrary 3-digit number, which is not bounded up to 599?

@quininer
Copy link
Contributor Author

quininer commented Oct 19, 2020

Many cases have been cited in #144 . About size, I will try to make it compact. (3 * 900) = 2700, this will be smaller than it is now.

@geota
Copy link

geota commented Nov 10, 2020

@quininer @Matthias247 is there an update to getting this merged? I have a vested interest to get this merged and could try to pick this up if @quininer is unable to finish off the PR, but I don't want to step on any toes.

@seanmonstar
Copy link
Member

I'd tentative consider merging, putting aside my strong feeling that servers shouldn't do this.

I think the issue of added binary size is valid. I don't know a great way to fix that, perhaps StatusCode::as_str should go away completely?

@quininer
Copy link
Contributor Author

quininer commented Nov 11, 2020

I think size issue should not block this PR, because this PR has made size smaller than before.

@geota
Copy link

geota commented Dec 2, 2020

@seanmonstar @Matthias247 can we figure out what the next steps on this PR should be to get this merged? Is size an issue? @quininer made the size smaller than before, so I feel that any other improvements here should be left to a future PR.

One thing to consider is folks who are trying to use this library in proxies where they have no control over what origins decide to return. I'd tentatively consider merging, putting aside my strong feeling that servers shouldn't do this.

@quininer
Copy link
Contributor Author

quininer commented Dec 8, 2020

Close by #443

@quininer quininer closed this Dec 8, 2020
@dekellum dekellum mentioned this pull request Dec 14, 2020
@quininer quininer deleted the custom-code2 branch December 25, 2020 07:30
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