Improve type hints for copy() and replace() in Request and Response.

[wRAR]

As discussed in #6127

[wRAR]

Without this line the type of redirect_request is inferred to be Any.

[kmike]

Could you please add a test for this case? I guess it's **kwargs handling?

[wRAR]

I think it's because it cannot know that kwargs passed to _build_redirect_request doesn't contain cls.

[wRAR]

Also, what test do you mean?

[kmike]

If kwargs are passed, what's the reveal_type of the result

[wRAR]

I've added tests that check that the result is Any unless cls was also passed.

[wRAR]

Not sure if it's a useful thing to check :)

[codecov]

Codecov Report

Merging #6143 (6b75d8f) into master (198f5cf) will decrease coverage by 0.07%.
Report is 9 commits behind head on master.
The diff coverage is 68.42%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #6143      +/-   ##
==========================================
- Coverage   88.89%   88.83%   -0.07%     
==========================================
  Files         161      161              
  Lines       11776    11818      +42     
  Branches     1913     1924      +11     
==========================================
+ Hits        10468    10498      +30     
- Misses        964      967       +3     
- Partials      344      353       +9     

Files	Coverage Δ
scrapy/downloadermiddlewares/httpcompression.py	91.07% <100.00%> (ø)
scrapy/downloadermiddlewares/redirect.py	94.38% <ø> (ø)
scrapy/http/response/__init__.py	92.47% <72.72%> (-2.88%)	⬇️
scrapy/http/request/json_request.py	90.90% <63.63%> (-9.10%)	⬇️
scrapy/http/request/__init__.py	92.92% <61.53%> (-4.88%)	⬇️

... and 8 files with indirect coverage changes

[kmike]

The approach looks good 👍

It seems the typing becomes more tricky now, especially as we have py.typed in scrapy now - it affects the user code, not just the internal type consistency. What do you think about adding some basic typing tests, at least for the important public interfaces like Response?

[wRAR]

What do you think should be tested, at least as a first step?

[kmike]

What do you think should be tested, at least as a first step?

I think we can do it gradually - start with the fixes, as compared to the previous implementation. E.g. that text_response.replace(...).text works properly for different scenarios (raises or doesn't raise mypy errors, as appropriate).

[wRAR]

Should the typing tests run on 3.8 or 3.12? We need different syntax for them.

[kmike]

Should the typing tests run on 3.8 or 3.12? We need different syntax for them.

Not sure. Is it because the error messages differ, or is it about something else? Can we rely on mypy error codes instead of exact error messages more?

[wRAR]

Is it because the error messages differ, or is it about something else?

Yes, mypy suggests A | B on newer Python but Union[A, B] on older Python.

Can we rely on mypy error codes instead of exact error messages more?

We can, though we run the test on a single Python version anyway, so we know which message will be produced.

[kmike]

I wonder if we should run it in the latest Python instead.

[wRAR]

Should we also run the typing check on the latest Python then, as I think they check the same thing? In that case we will stop checking (and so supporting) that the type hints work as expected on older Python versions and I thought it's a nice thing to support.

[wRAR]

(I mean, for users that use Scrapy on older Python versions and run mypy on their code)

[kmike]

@wRAR

We can, though we run the test on a single Python version anyway, so we know which message will be produced.

This works. Do you think thought that the more detailed matching of the error messages help us to catch more issues, as compared to just checking the mypy error code?

[wRAR]

Do you think thought that the more detailed matching of the error messages help us to catch more issues, as compared to just checking the mypy error code?

Yeah, I think error codes can be too broad to be useful.