Skip to content

Change test Client headers parameter to Mapping[str, str] #1534

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

intgr
Copy link
Collaborator

@intgr intgr commented Jun 6, 2023

NOTE: Some changes that were originally part of this PR have already been merged in #1537.

  • Changed headers= parameter type from Mapping[str, Any] to Mapping[str, str]
  • Changed headers attribute from dict[str, Any] to Mapping[str, str] | None

Related issues

@intgr
Copy link
Collaborator Author

intgr commented Jun 6, 2023

Ping @Alexerson as author of #1529

@sobolevn
Copy link
Member

sobolevn commented Jun 6, 2023

Changed headers dict value type from Any to str

I would say it is object, not Any or str. Because object.__str__ is defined.

@intgr
Copy link
Collaborator Author

intgr commented Jun 6, 2023

I would say it is object, not Any or str. Because object.__str__ is defined.

Passing anything other than str causes request.headers[x] to return incorrectly typed values in a view function being tested.

    x = request.headers["My-header"]
    reveal_type(x)  # note: Revealed type is "builtins.str"
image

@Alexerson
Copy link
Contributor

Thanks or the other changes.
As for the str vs Any, I'm not quite sure yet.

There is indeed no casting happening in the code in Django, so the current typing is matching the behavior. I can see that the real HttpRequest object is constructing the headers with values from META, and this parameter right now also have the typing dict[str, Any].

I will try to find more info to see if that's valid, or if we should tighten the type, but in that case, I'm assuming it would need to be everywhere.

@intgr
Copy link
Collaborator Author

intgr commented Jun 6, 2023

Apologies in advance for the following rant, but I just find this so weird. When I suggested changing Any to str in #1529 (comment) and in this PR, two maintainers were immediately jumping in to voice their preference of loose Any/object typing, even with no verification whether it actually works correctly.

When I point out this actually causes issues, there is total radio silence, as if there is silent disagreement.

It's not the first time either. I've noticed this pressure to loosen up types in other discussions too, where I've been the only lone voice to advocate for avoiding Any.


During the last decade or so, it's finally begun to sink in to many programming communities that loose typing causes trouble. Not just Python, but also TypeScript, adoption of generics in Java and Go, null annotations in Java, and the push for more expressive type systems in general. This is what drew me into using mypy and contributing to django-stubs.

Yet in this one project that aims to bring typing to Django, people regularly advocate for loosening types to accept "yolo whatever". Why is this? I honestly find this frustrating.

HTTP headers are a concept defined in the HTTP text-based protocol. They are mappings of strings to strings. Why should django-stubs accept anything other than strings for headers? What does it mean for a header to have value of type int or list[int] or Ellipsis or ModuleType?

If someone wants to pass a number, or string-value of other object, as an HTTP header value, I think changing some_val to str(some_val) is actually an improvement, "Explicit is better than implicit" and all that.

Maybe there is a case to be made to accept implicit conversion from int to str (if this implicit conversion were done in Django), we could use int | str. But the vast majority of values accepted by Any are in reality mistakes that make no sense, and where type checkers could actually help people find and avoid issues.

@Alexerson
Copy link
Contributor

Even though my opinion is not completely made on the topic yet, I agree with you that types should help us write better Python, not just make us happy because we feel we've added something to our codebase.
But considering my limited contribution so far, I think that's gonna be the limit to my input.

Regarding headers: I tend to agree with you, but if that's OK with you, I'll try to look at the specs to see if there is any use case for something other than str that we are overlooking (either in WSGI or ASGI).

@Alexerson
Copy link
Contributor

Alexerson commented Jun 6, 2023

OK, so reading a bit more how headers are constructed, I see that the values are indeed strings for ASGI: in Django, it’s done here. (I checked other implementations, and it seems to be consistent with this)

For WSGI, I wonder if we should use wsgiref.types.WSGIEnvironment for the WGSIRequest.environ parameter − that’s what’s used for the META parameter.

So your PR is definitively valid for async one. I’m trying to read more about WSGIEnvironment to help decide what it should be for the "normal" one, because I don’t think we want to use that in tests.

@Alexerson
Copy link
Contributor

Alexerson commented Jun 6, 2023

Well… I guess we might want to stick with dict[str, Any]
See https://github.com/python/cpython/blob/3907de12b57b14f674cdcc80ae64350a23af53a0/Lib/wsgiref/types.py#L29

Edit: and see also python/typeshed#1745

The thing is, META can be an Any, but headers per se are most likely only strings. But I can’t find the info if this is really a spec, or if it’s an implementation detail.

@sobolevn
Copy link
Member

sobolevn commented Jun 6, 2023

We can reuse this type :)

@intgr
Copy link
Collaborator Author

intgr commented Jun 6, 2023

Right now request header values are typed as str, see HttpHeaders type (which is used for HttpRequest.headers).
https://github.com/typeddjango/django-stubs/blob/master/django-stubs/http/request.pyi#L24

If you insist that header values are Any because they come from WSGIEnvironment, then you should also change that.

But I think that would be a big mistake.

@Alexerson
Copy link
Contributor

I might be wrong, but CaseInsensitiveMapping[str]) simply means that keys are str. It doesn’t say anything about values. No?

@Alexerson
Copy link
Contributor

Ah actually you’re right, I read it this morning while doing my search, and thought that this is what it was, but it’s indeed enforcing that values are str (the keys being str is enforced in the class def).

So, yeah, we should probably do the same thing in the test part.

@intgr
Copy link
Collaborator Author

intgr commented Jun 8, 2023

I split out the non-controversial changes as a separate PR in #1537.

This discussion is now focused on whether header values should be typed str or Any.

@intgr intgr added the discussion needed Issues or PRs that require further discussion or a decision to be made label Jun 14, 2023
Copy link
Contributor

@adamchainz adamchainz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When I point out this actually causes issues, there is total radio silence, as if there is silent disagreement.

Apologies on my end. I have had little time for open source in the last few weeks. Back now.

Thank you for checking that headers does not cast to str. I did indeed presume it did at some point between the client and response objects, but clearly not.

Using str LGTM!

Yet in this one project that aims to bring typing to Django, people regularly advocate for loosening types to accept "yolo whatever". Why is this? I honestly find this frustrating.

I would only like to loosen types where it will help adoption. The dream would that be adding django-stubs to an existing, mature, tested project would only reveal true errors and not require extra narrowing/casting to make things work. That was my intent here, but I could have tested before asking.

@intgr intgr force-pushed the update-test-client-headers-types branch from 84bfa0f to 5ed48b2 Compare June 27, 2023 20:04
@intgr intgr changed the title Update test Client headers parameter types Change test Client headers parameter to Mapping[str, str] Jun 27, 2023
@intgr
Copy link
Collaborator Author

intgr commented Jun 27, 2023

I have rebased and updated this PR.

@intgr intgr removed the discussion needed Issues or PRs that require further discussion or a decision to be made label Jun 27, 2023
@intgr intgr self-assigned this Dec 26, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

4 participants