Skip to content

Conversation

@hoefling
Copy link
Contributor

Signed-off-by: Oleg Hoefling [email protected]

I have made things!

This is #1252, reintroduced. Description copied for the sake of completeness:

The redirect_chain attribute was added in #1124. At runtime, the response only has this attribute set when the request was made with follow=True:

In [1]: from django.test import Client

In [2]: c = Client()

In [3]: c.get('/spam', follow=True).redirect_chain
Not Found: /spam
2022-11-12 15:42:28 [WARNING ] django.request :: Not Found: /spam
Out[3]: []

In [4]: c.get('/spam', follow=False).redirect_chain
Not Found: /spam
2022-11-12 15:42:33 [WARNING ] django.request :: Not Found: /spam
---------------------------------------------------------------------------
AttributeError                            Traceback (most recent call last)
Input In [4], in <cell line: 1>()
----> 1 c.get('/spam', follow=False).redirect_chain

AttributeError: 'HttpResponseNotFound' object has no attribute 'redirect_chain'

This PR replicates this behaviour.

Related issues

Refs #1252

Copy link
Member

@sobolevn sobolevn left a comment

Choose a reason for hiding this comment

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

The same is for all other overloads.

Do you agree?

def request(self, **request: Any) -> _MonkeyPatchedWSGIResponse: ...
def get( # type: ignore
@overload # type: ignore
def get(
Copy link
Member

Choose a reason for hiding this comment

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

I think we can remove this overload, it is the same as bool

Copy link
Contributor Author

@hoefling hoefling Nov 16, 2022

Choose a reason for hiding this comment

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

@sobolevn this will cause a false positive tho for the default value, e.g. Client().get('foo').response_chain will be inferred as existing since the follow: Literal[True] case matches and I can't put it behind the broader follow: bool case. The client_follow_flag test I added should also fail when the first override is deleted.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hmm. Ordinarily you would remove the default value (= ...) from the argument in one of the overloads, like

@overload
def get(self, path: str, data: Any = ..., follow: Literal[True], secure: bool = ..., **extra: Any) -> _MonkeyPatchedWSGIResponseRedirect: ...
#                                         ^^^^^^^^^^^^^^^^^^^^^
@overload
def get(self, path: str, data: Any = ..., follow: bool = ..., secure: bool = ..., **extra: Any) -> _MonkeyPatchedWSGIResponse: ...

But here it's not allowed because it follows another argument with default data.

I'm not aware of any tricks to make it work. May indeed have to use 3 overloads.

response = client.delete('foo', follow=True)
reveal_type(response.redirect_chain) # N: Revealed type is "builtins.list[Tuple[builtins.str, builtins.int]]"
x: bool
response = client.delete('foo', follow=x)
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should allow bool? I think the main usecase for this is in unit-tests. They will be checked in all cases, but we can just make things harder for users by producing errors (and possibly false-positives).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@sobolevn sorry, I cannot follow you. Or do you mean "not allow bool" instead? Only boolean literals?

Copy link
Member

Choose a reason for hiding this comment

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

Sorry :(

I mean that using bool should not produce this error. Because users will get a unittest failure if it is not True anyway. And I don't want to have false-positives in test code.

Is it better now? 😊

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