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
oauthlib: typing for gettattr overrides #10613
base: main
Are you sure you want to change the base?
Conversation
oauthlib maintains many attributes within a `self._params` dict.
This comment has been minimized.
This comment has been minimized.
This is already handled by the But you should probably change them to read only properties, since there's no |
Also it seems to me like all of these should actually be |
Let's put this on hold. I'm going to open a ticket with the project, as the overall project documentation and code teaches away from these being purely decode http(s) request params in several spots. |
@jvanasco Sure, you can could technically pass in an iterable of (key, value) tuples or a mapping as the body and set So I'm fairly confident the value will always end up as a string, with the default being |
@Daverball I've opened an issue against the package here: oauthlib/oauthlib#856 TLDR: in practice, the oauthlib request object will eventually have objects on the I think the project grew over time, and some of the infrastructure evolved or was repurposed. Within the context of that file, your comments are correct - however I think that file is incorrect and several items need to be turned into explicit attributes. For example:
And again we have https://github.com/oauthlib/oauthlib/blob/4a7db54f005686128102d7f7ac5c3d783c244669/oauthlib/oauth2/rfc6749/request_validator.py#L83-L85 :
So basically the rest of the library teaches away from the doctstring and obvious interpretation in that file. This is the sort of problem that typing surfaces. |
Yeah you're right, I've glossed over the part where |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These are all read-only (there is no __setattr__
), so they would be better typed with @property
, e.g.
@property
def access_token(self) -> Any: ...
That's a great idea. I'll have to audit the code to ensure these all match that pattern and adjust the PR accordingly. This package doesn't have a standardized system or style guide, and typing+tests have been surfacing some oddities. |
This comment has been minimized.
This comment has been minimized.
I've gone through the code to find all the potential Instead of typing to I also used this format: instead of as I could not find a preferred style, and would be happy to change if requested Notes: These use Multiple types: Not used anyhere: New: |
This comment has been minimized.
This comment has been minimized.
According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉 |
Thank you all for the review and fixing the Union + style changes. I'm copy/pasting between far too many windows and environments trying to handle this. After testing out the 1- I maintain an integration package from the Pyramid framework. The oauthlib package itself is quite barebones and expects to be subclassed for integration - it basically defines an API that must be met. After type-checking my code against this version, I'm not sure what the best way to handle this is, but this approach doesn't seem to improve typing support. Edit: I have also incorrectly typed the attributes set in init: they should not have Optionals:
|
Sorry for the lack of response here. At the minimum, a few changes are necessary to make CI pass. It might also be helpful to add test cases covering your usage to make sure things work correctly for typical usage. |
No worries. This can be closed. I previously opened a ticket on oauthlib here oauthlib/oauthlib#856 There are several issues with oauthlib related to typing that are best solved in that project. |
First: I am not sure this is the correct way to handle this situation. If a better technique is recommended, please educate me.
Problem:
oauthlib
manages many (most) attributes for theoauthlib.common.Request
object within a dict namedself._params
For reference: https://github.com/oauthlib/oauthlib/blob/master/oauthlib/common.py#L360-L401
This creates issues for typing projects that rely on this, as these attributes don't actually exist on that class.
In this PR: I made a section for these managed attributes, and then set them to Any