[9643] Agent.request(method) must be bytes

[twm]

Raise TypeError when the method argument of Agent.request() isn't bytes.

It would be more usable to make it accept str as well, but that would require changes to IAgent.request() which is explicit about it being bytes. Since third party IAgent implementations are actually a thing, I don't think that we should make that change.

Contributor Checklist:

• There is an associated ticket in Trac. https://twistedmatrix.com/trac/ticket/9643#comment:2
• The changes pass minimal style checks (see: https://twistedmatrix.com/trac/wiki/TwistedDevelopment#GettingYourPatchAccepted )
• I have created a newsfragment in src/twisted/newsfragments/ (see: https://twistedmatrix.com/trac/wiki/ReviewProcess#Newsfiles )
• I have updated the automated tests.

[moshez]

Thank you for making Twisted web client more usable!

Approved, but consider the suggestion for improvement

[moshez]

Wouldn't it be better to do it in _AgentBase._requestWithEndpoint, so that ProxyAgent won't have literally the same bug?

This is not a blocker, but should be considered.

[exarkun]

If it's not a blocker, a new ticket for that issue should at least be filed.

[adiroiban]

Why not make it a blocker, and also fix the ProxyAgent ?

[exarkun]

At this point, probably because moshez already approved the PR and it is contributor unfriendly to have conflicting requirements imposed on their contributions. If there's an issue here, the reasonable next step is probably to discuss with moshez why he didn't think this should be a blocker and come to some agreement for future work.

FWIW, the PR as-is seems like a specific, concrete improvement to a feature of Twisted. It is not incomplete because it does not also address ProxyAgent. It would be great to also fix ProxyAgent but no harm is done by taking on that work separately.

[adiroiban]

Thanks. I agree.

[adiroiban]

just a comment... instead of raising an error, why not try to recover from the error and convert the method using 'ASCII' encoding?

I expect that in most of the people are just calling it using something like u'GET' and not using non-ASCII method names.

[exarkun]

just a comment... instead of raising an error, why not try to recover from the error and convert the method using 'ASCII' encoding?

I expect that in most of the people are just calling it using something like u'GET' and not using non-ASCII method names.

Because this would introduce failures that depend on the value instead of on the type. Then, these failures would happen based on program input instead of program construction and be harder to predict, track down, and fix.

[adiroiban]

Because this would introduce failures that depend on the value instead of on the type.

Fair point... is harder to detect value based errors.

My understanding is that HTTP method name is a "token" so is safe to assume that you will always receive ASCII characters.

CHAR           = <any US-ASCII character (octets 0 - 127)>

token          = 1*<any CHAR except CTLs or separators>

I was hoping that in this way it will be bit easier to migrate to py3.

[exarkun]

Because this would introduce failures that depend on the value instead of on the type.

Fair point... is harder to detect value based errors.

My understanding is that HTTP method name is a "token" so is safe to assume that you will always receive ASCII characters.

CHAR           = <any US-ASCII character (octets 0 - 127)>

token          = 1*<any CHAR except CTLs or separators>

I was hoping that in this way it will be bit easier to migrate to py3.

That's always a nice thing to hope for but I think in reality it will make it harder to migrate to Python 3 and then also harder to maintain the resulting Python 3 code.

[twm]

just a comment... instead of raising an error, why not try to recover from the error and convert the method using 'ASCII' encoding?

That's always a nice thing to hope for but I think in reality it will make it harder to migrate to Python 3 and then also harder to maintain the resulting Python 3 code.

It absolutely would make it easier to port code that uses Agent to Python 3 if this argument were str, but it's too late. IAgent already requires bytes, and we can't (or at least, shouldn't) require that every implementation handle both. Besides, there is another layer that can hide this rough edge from the end user: treq.

Because this would introduce failures that depend on the value instead of on the type. Then, these failures would happen based on program input instead of program construction and be harder to predict, track down, and fix.

Actually value-dependent failures here are already possible because the implementation doesn't actually verify that method confirms to the token character set. For example you can pass a nonsensical method like b'GET POST', likely resulting in a 400 response or connection drop from the server.

I do think that checking the type is enough to avoid much user confusion.

[twm]

Thank you for the review, @moshez! I did as you suggested with ProxyAgent and added a test for it.

[moshez]

You're welcome, and since it's already approved, you can merge at will :)