Skip to content

OrdinalIgnoreCase is unnecessary #42

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

Merged
merged 1 commit into from
Jan 27, 2016
Merged

Conversation

benaadams
Copy link
Contributor

https://www.techempower.com/benchmarks/#section=code&hw=peak&test=json

General requirements
3. Specific characters and character case matter.

https://www.techempower.com/benchmarks/#section=code&hw=peak&test=json
> General requirements
3. Specific characters and character case matter.
@DamianEdwards
Copy link
Member

Technically, I believe they're referring to casing in responses, not URIs. The language around URIs is "The recommended URI is /json"

@benaadams
Copy link
Contributor Author

yeah but linux stuff is generally case sensitive so bbc news is linux; you can tell as if you go here its ok

http://www.bbc.co.uk/news/scotland but if you capitalize it 404s http://www.bbc.co.uk/news/Scotland

If you are being compared to linux frameworks on linux; should use the same rules? Windows is just generally more forgiving.

@DamianEdwards
Copy link
Member

Sure, but the other examples I tried in the TechEmpower benchmarks didn't care about casing AFAICT. All that said, we probably don't need to have this line anyway (everything here is controlled).

@benaadams
Copy link
Contributor Author

Ordinal has improves dotnet/coreclr#2825

@DamianEdwards
Copy link
Member

Wow, that's cool. We'll get that benefit whether we remove the case-insensitive check or not though.

@DamianEdwards
Copy link
Member

Bah, OK you win, removing code is good 😄

DamianEdwards added a commit that referenced this pull request Jan 27, 2016
OrdinalIgnoreCase is unnecessary
@DamianEdwards DamianEdwards merged commit 0ac3574 into aspnet:dev Jan 27, 2016
@benaadams
Copy link
Contributor Author

We'll get that benefit whether we remove the case-insensitive check or not though.

Not if you fall through to next item

@DamianEdwards
Copy link
Member

Oh good point, although that's less of an issue with my latest changes (only enable specific scenarios).

@benaadams
Copy link
Contributor Author

And.... just for completeness https://tools.ietf.org/html/rfc7230#section-2.7.3

The scheme and host are case-insensitive and normally provided in lowercase; all other
components are compared in a case-sensitive manner.

However with controllers and methods being camel case and paths generally being lowercase (ignoring @shanselman's wild blog post paths); probably would want to be case insensitive for general aspnet path testing.

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.

3 participants