httpx icon indicating copy to clipboard operation
httpx copied to clipboard

improve types for RequestData

Open adriangb opened this issue 3 years ago • 3 comments
trafficstars

Context: https://github.com/orgs/encode/teams/maintainers/discussions/2/comments/15 #2266

adriangb avatar Oct 11 '22 23:10 adriangb

@tomchristie how would you feel about running pyright in CI so we can use it for informational purposes, maybe using it’s typing score as a rough metric for “is this PR improving things for users” without any sort of enforcement?

adriangb avatar Oct 12 '22 01:10 adriangb

I'd suggest that if we're going to start making type changes like this and https://github.com/encode/httpx/pull/2266, then we do so with a more precise approach.

Removing TypeError checks.

Personally I'm probably against this, but more importantly let's discuss that as a separate issue.

Stricter type checking with mypy.

I'd be okay with pull requests that resolve mypy strict type checking issues. I'd suggest that pull requests include the mypy httpx --strict output before and after the change, and only make changes that resolve the output. PRs should either only deal with a single module at a time, or should only deal with a single mypy error class at a time.

Enforced stricter type checking with mypy.

I don't think we need to consider turning on mypy --strict in our CI at the moment. If we're getting close to full compliance, then it'd be worth looking at enabling that.

Stricter type checking with pyright.

I'd prefer we avoid adding a node dependancy to our CI. I'd be interested in seeing what issues pyright currently indicates, and determining if mypy --strict covers the same set of cases or not.

lovelydinosaur avatar Oct 12 '22 10:10 lovelydinosaur

@tomchristie I requested re-review since I removed the removal (so restored?) the runtime type checks. This is still touching multiple modules but it is dealing only with a single type (RequestData).

I think that the lack of changes to the tests (besides adding a check for a case that would have previously been a false positive) are a good indication that this is not changing any runtime behavior despite the internal refactoring.

adriangb avatar Nov 29 '22 05:11 adriangb

Closing as stale

lovelydinosaur avatar Mar 01 '24 15:03 lovelydinosaur