Add :downcase_request_headers option to HTTP1.connect

elixir-mint / mint

Functional HTTP client for Elixir with support for HTTP/1 and HTTP/2 🌱

Apache License 2.0

1.36k stars 112 forks source link

Add :downcase_request_headers option to HTTP1.connect #399

Closed DunyaKokoschka closed 7 months ago

DunyaKokoschka commented 1 year ago

i hear you want not lower header. i give you not lower header. but also i make default headers not lower if you pass in not lower option. alexi say this change is slow. i tell alexi if you care about slow why using elixir.

https://github.com/elixir-mint/mint/issues/256

whatyouhide commented 1 year ago

Do you know if this is a behavior that we should have for HTTP/2 as well?

ericmj commented 1 year ago

Do you know if this is a behavior that we should have for HTTP/2 as well?

I would say no until proven otherwise. AFAIK all HTTP/2 implementers properly handle header casing.

DunyaKokoschka commented 1 year ago

I've added some commits which address the comments. The name of the option is currently case_sensitive_headers. And I added the docs for the option to the Mint.HTTP1.connect method. I'm not sure if this is the correct place. I wasn't so sure about this comment: case_sensitive_headers

Also, locally the twitter integration test is failing for me with a 302 instead of a 200.

coveralls commented 1 year ago

Pull Request Test Coverage Report for Build 17d2697acfe911fefda4291ee5a04f392057aa6b-PR-399

Details

-1 of 55 (98.18%) changed or added relevant lines in 6 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage increased (+0.08%) to 87.509%

Changes Missing Coverage	Covered Lines	Changed/Added Lines	%
lib/mint/core/headers.ex	18	19	94.74%
<!--	Total:	54	55	98.18%	-->

Totals
Change from base Build ce8102b57a9bf8b4159cd0096b31d08b5d70ad09:	0.08%
Covered Lines:	1268
Relevant Lines:	1449

💛 - Coveralls

ericmj commented 7 months ago

@whatyouhide I rebased and did some clean up. Do you think we are good to merge this?