[chripede]

#240

[cdeler]

Hello @chripede

You changes are discarded by CI. It happens because some of our files are autogenerated, for example httpcore/_sync/http2.py is generated from httpcore/_async/http2.py. You can do it locally using the script named lint (it is in the project repository scripts/lint)

You can make the changes in the async file, and then update the sync using the lint script

Also, there is the second script named script/test, which can help you to run CI locally (just to check if CI is ok locally)

More information you can find on the project website: testing-and-linting

[florimondmanca]

Looking good! Left a suggestion to add an explanatory comment.

As @cdeler hinted, the workflow in this repo is to A/ edit files in the _async/ directory, then B/ run scripts/lint to generate the sync version. If following this workflow you should be able to get CI back in check. :-)

[chripede]

For some reason Black formats the async file different than the sync file. Fixing one linting problem introduces another. Any help would be appreciated ;)

[chripede]

Nevermind the last comment. Fixing by hand worked and Black doesn't return it to a one-liner.

[cdeler]

Looks good for me 🍦

[florimondmanca]

Small tweak (almost a nitpick, I'm aware), but this is looking good! Any chance we could add a unit test?

[chripede]

I would create a unit test, but I am unable to run the current set of tests

[florimondmanca]

@chripede That's hum, interesting. It looks like you're showing the stdout output (pytest -s). What does the pytest final output look like — which tests are failing?

[florimondmanca]

A unit test would be fab but I'm good with merging this as-is too, so, approving.

[cdeler]

@florimondmanca
I'm merging this ticket as it's approved.
I think it's ok to add new tests in an extra PR linked to #240

[lovelydinosaur]

I'd prefer if we didn't change the pseudo-header behaviour as part of this PR.

There's some complexities here that'd need further discussion. Since this change isn't necessary for the rest of the pull request to make sense, let's treat anything here independently.

[chripede]

I agree that it isn't part of this fix, but I changed it because I tried adding ":authority" to the headers and got an exception because of duplicate headers.

[lovelydinosaur]

Sure thing. If we want to tackle that let's do so separately tho.

For instance, if we are going to strip pseudo headers like this, then we'd probably also want to do the same for HTTP/1.1 codepaths.

It's part of a more general conversation about "which header values should be treated as valid".

[lovelydinosaur]

This case shouldn't override an explicit Host header, so I think there's a bit of tweaking needed here.

[chripede]

It doesn't. At this point, authority has the value of an explicit Host header.

[lovelydinosaur]

Right, tho we don't want to append the port here.