Skip to content

Conversation

@lepture
Copy link

@lepture lepture commented Oct 28, 2020

Since UNSET is widely used in lots of functions' parameters. When people want to subclass and rewrite a method, or create a function to call httpx, they will need to use this UNSET to define their own parameters too.

A simple example:

def request_token(client, auth=UNSET, **kwargs):
    url = 'https://example.com/token'
    return client.get(url, auth=auth, **kwargs)

Here are some real examples:

  1. https://github.com/lepture/authlib/blob/v0.15.2/authlib/integrations/httpx_client/oauth2_client.py#L117
  2. https://github.com/lepture/authlib/blob/v0.15.2/authlib/integrations/httpx_client/oauth2_client.py#L136

Since UNSET is widely used in lots of functions' parameters. When people want to subclass and rewrite a method, or create a function to call httpx, they will need to use this UNSET to define their own parameters too.
@florimondmanca
Copy link
Contributor

florimondmanca commented Nov 7, 2020

@lepture Hi,

I think it's an interesting point, and I don't think I have an answer right now. I do agree that having this specific type leak through is a bit of a smell, eventually. What would a world in which each library has its own UNSET type, to account for the fact that undefined doesn't exist in Python? 😅

But for now, for the specific use case of the two code examples you listed, it doesn't seem like we need to solve it just yet. In your 2 examples, and also in the snippet in this PR description, it doesn't struck me why the auth has to be exposed outside of **kwargs? There are no special operation performed on auth, it's just passed as self.post(..., auth=auth, **kwargs).

@florimondmanca
Copy link
Contributor

Closing pre-emptively since I do agree on the problem (it's likely that UnsetType shouldn't leak to public methods), but exposing UNSET is not the right solution I believe. We can still discuss alternatives, though.

@lepture
Copy link
Author

lepture commented Nov 7, 2020

Yes, my given examples are not proper. Here is a proper one: https://github.com/lepture/authlib/blob/2af1b9f2c81d5cb847491e96cf762f69215fcf2d/authlib/integrations/httpx_client/oauth2_client.py#L84

There is a check if auth is defined, if not it should attach one.

@lepture
Copy link
Author

lepture commented Nov 7, 2020

And also, I have a suggestion, if there is a breaking change, please don’t release it with a bugfix version.

@florimondmanca
Copy link
Contributor

florimondmanca commented Nov 7, 2020

Gotcha. ;-) Are you referring to a specific issue that's recently struck you?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants