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

Header type equality comparison #1418

Closed
psibi opened this issue Jul 13, 2017 · 6 comments
Closed

Header type equality comparison #1418

psibi opened this issue Jul 13, 2017 · 6 comments

Comments

@psibi
Copy link
Member

@psibi psibi commented Jul 13, 2017

The equality comparison is case insensitive according to the HTTP specs. Right now, we don't make that distinction.

@snoyberg
Copy link
Member

@snoyberg snoyberg commented Jul 13, 2017

Header keys are wrapped in the CI type constructor, which ensures a case insensitive comparison. Is there a specific counterexample?

@psibi
Copy link
Member Author

@psibi psibi commented Jul 13, 2017

@snoyberg I think this Header is not wrapped in CI type constructor: https://www.stackage.org/haddock/lts-8.22/yesod-core-1.4.35/Yesod-Core.html#t:Header

@psibi
Copy link
Member Author

@psibi psibi commented Jul 13, 2017

I think it would be better to just wrap it in CI type constructor instead of doing Eq instance fix.

@paul-rouse
Copy link
Contributor

@paul-rouse paul-rouse commented Jul 13, 2017

At the moment the code in #1417 would still need to be changed to use Eq on Header - does this actually go any wider than that code, or would it be best just to use CI for case-insensitivity there, for that one comparison?

@psibi
Copy link
Member Author

@psibi psibi commented Jul 13, 2017

I would vote for using CI for case-insensitivity there as that's the most easiest thing to do. :)

@snoyberg
Copy link
Member

@snoyberg snoyberg commented Jul 13, 2017

@psibi Up until now, there has been no comparison performed there that I'm aware of. 20/20 hindsight, it would make sense to use CI ByteString, but I'd rather not break the API for this.

@psibi psibi mentioned this issue Jan 15, 2018
@snoyberg snoyberg closed this in 915d9e2 Feb 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
3 participants
You can’t perform that action at this time.