Open aliev opened 2 weeks ago
:warning: Please install the to ensure uploads and comments are reliably processed by Codecov.
Attention: Patch coverage is 91.44737%
with 13 lines
in your changes missing coverage. Please review.
Project coverage is 93.42%. Comparing base (
533d8d8
) to head (09998b0
).
:exclamation: Your organization needs to install the Codecov GitHub app to enable full functionality.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Really love the changes in this upcoming release. Excited to see it completed. If there's anyway I can assist lmk, eager to use this in an upcoming project of mine.
Really love the changes in this upcoming release. Excited to see it completed. If there's anyway I can assist lmk, eager to use this in an upcoming project of mine.
Cool! I’d be happy to see a code review.
Additionally, in version 2, I plan to include working examples directly in the repository, possibly in an examples
directory.
I was also thinking about a simple working server that could be started with the command:
python -m aioauth.http
Love the generics simplification/improvement. The separation of storage into separate classes is a great addition too.
Thanks :) There were too many generics in the first version, which caused issues with mypy and led to type checks being ignored.
Now I’m using a generic only for User
type (which defaults to Any
) :)
Another suggestion up for debate: a lot of the storage functions understandably have a lot of arguments being passed into them meaning the function signatures can get almost egregiously long. for example: create_authorization_code
https://github.com/aliev/aioauth/blob/release/2.0.0/aioauth/storage.py#L90C5-L101C28
async def create_authorization_code(
self,
request: Request[UserType],
client_id: str,
scope: str,
response_type: ResponseType,
redirect_uri: str,
code_challenge_method: Optional[CodeChallengeMethod],
code_challenge: Optional[str],
code: str,
**kwargs,
) -> AuthorizationCode:
...
could we potentially use dataclasses to pass all of the arguments to make the signatures themselves much smaller?
@dataclass
class CreateAuthCodeReq(Generic[UserType]):
oauth_request: Request[UserType]
client_id: str
scope: str
response_type: ResponseType
redirect_uri: str
code_challenge_method: Optional[CodeChallengeMethod]
code_challenge: Optional[str]
code: str
kwargs: Dict[str, Any]
async def create_authorization_code(self, request: CreateAuthCodeReq) -> AuthorizationCode:
...
just an idea.
Another suggestion up for debate: a lot of the storage functions understandably have a lot of arguments being passed into them meaning the function signatures can get almost egregiously long. for example:
create_authorization_code
https://github.com/aliev/aioauth/blob/release/2.0.0/aioauth/storage.py#L90C5-L101C28async def create_authorization_code( self, request: Request[UserType], client_id: str, scope: str, response_type: ResponseType, redirect_uri: str, code_challenge_method: Optional[CodeChallengeMethod], code_challenge: Optional[str], code: str, **kwargs, ) -> AuthorizationCode: ...
could we potentially use dataclasses to pass all of the arguments to make the signatures themselves much smaller?
@dataclass class CreateAuthCodeReq(Generic[UserType]): oauth_request: Request[UserType] client_id: str scope: str response_type: ResponseType redirect_uri: str code_challenge_method: Optional[CodeChallengeMethod] code_challenge: Optional[str] code: str kwargs: Dict[str, Any] async def create_authorization_code(self, request: CreateAuthCodeReq) -> AuthorizationCode: ...
just an idea.
Ah, valid point! How about using the Unpack feature? It should work in Python 3.9 if we include typing_extensions
.
I just tested this solution, and it works perfectly:
if sys.version_info >= (3, 11):
from typing import Unpack, NotRequired
else:
from typing_extensions import Unpack, NotRequired
class AuthorizationCodeKwargs(Generic[UserType], TypedDict):
request: Request[UserType]
client_id: str
scope: str
response_type: ResponseType
redirect_uri: str
code_challenge_method: Optional[CodeChallengeMethod]
code_challenge: Optional[str]
code: str
nonce: NotRequired[Optional[str]]
...
class AuthorizationCodeStorage(Generic[UserType]):
async def create_authorization_code(
self,
**kwargs: Unpack[AuthorizationCodeKwargs],
) -> AuthorizationCode:
Ah, valid point! How about using the Unpack feature? It should work in Python 3.9 if we include typing_extensions.
Oh super neat. I had no idea that was a thing. That sounds like a great solution :)
Ah, valid point! How about using the Unpack feature? It should work in Python 3.9 if we include typing_extensions.
Oh super neat. I had no idea that was a thing. That sounds like a great solution :)
it also helps with the autocomplete in the IDE:
Ah, valid point! How about using the Unpack feature? It should work in Python 3.9 if we include typing_extensions.
Oh super neat. I had no idea that was a thing. That sounds like a great solution :)
Alright, I pushed the fix: https://github.com/aliev/aioauth/pull/106/commits/117397cfedb0eeab76bb96956eb1f5412ee5097b
@aliev
Alright, I pushed the fix: 117397c
Hate to be that guy but I think I was wrong about the dataclasses/unpack thing being a good idea. Working on the examples today and I think the benefits are outweighed with the potential cons of switching to using unpack.
A large part of my intention was making it so the function signature wasnt so many characters long (preferably less than 80) so it looks nicer and more concise all on one line when defining storage interfaces but the signatures are still pretty long with reasonable names for the TypeDict definitions reguardless:
async def create_authorization_code(self, **kwargs: Unpack[CreateAuthorizationCodeArgs[UserType]]) -> AuthorizationCode:
raise NotImplemented
# or trying to follow 80 chars forces you to do this:
async def create_authorization_code(self,
**kwargs: Unpack[CreateAuthorizationCodeArgs[UserType]]
) -> AuthorizationCode:
raise NotImplemented
This has additional downsides as well unfortunately because it obscures the actual arguments being passed behind the TypeDict definition that's only visible through IDE intellisense and likely will force the developer to go and then lookup the associated key word arguments.
That said I'm happy to talk about it if you have any thoughts on the matter.
@aliev, I finished a basic example for fastapi intended to be a minimum viable product, lmk what you think.
Just FYI, I really hate starlettes/fastapi's default session management and it was making things way more complicated, so I ended up using my own session-library in the example.
If you like it, i can potentially post a PR or something.
@aliev
Alright, I pushed the fix: 117397c
Hate to be that guy but I think I was wrong about the dataclasses/unpack thing being a good idea. Working on the examples today and I think the benefits are outweighed with the potential cons of switching to using unpack.
A large part of my intention was making it so the function signature wasnt so many characters long (preferably less than 80) so it looks nicer and more concise all on one line when defining storage interfaces but the signatures are still pretty long with reasonable names for the TypeDict definitions reguardless:
async def create_authorization_code(self, **kwargs: Unpack[CreateAuthorizationCodeArgs[UserType]]) -> AuthorizationCode: raise NotImplemented # or trying to follow 80 chars forces you to do this: async def create_authorization_code(self, **kwargs: Unpack[CreateAuthorizationCodeArgs[UserType]] ) -> AuthorizationCode: raise NotImplemented
This has additional downsides as well unfortunately because it obscures the actual arguments being passed behind the TypeDict definition that's only visible through IDE intellisense and likely will force the developer to go and then lookup the associated key word arguments.
That said I'm happy to talk about it if you have any thoughts on the matter.
Thanks for pointing that out. I understand the downsides of using Unpack
, especially when it comes to readability.
I’ve thought about an alternative approach: we could explicitly define the primary arguments like request
and client_id
that are used in every method, and leave the rest as kwargs
. For example:
async def create_authorization_code(
self,
request: Request[UserType],
client_id: str,
**kwargs: Unpack[CreateAuthorizationCodeArgs[UserType]],
) -> AuthorizationCode:
raise NotImplemented
This way, the method signature becomes more explicit and the core arguments are easier to understand at a glance.
Regarding the length of the type hint for kwargs
, I think there might be room to make it more concise. Ultimately, I believe it's up to the library user to decide which approach works best for them—whether to use kwargs
with Unpack
, or to explicitly list all the arguments.
I’ve thought about an alternative approach: we could explicitly define the primary arguments like request and client_id that are used in every method, and leave the rest as kwargs.
That makes a lot of sense. I like the compromise, good idea :)
@aliev, I finished a basic example for fastapi intended to be a minimum viable product, lmk what you think.
Just FYI, I really hate starlettes/fastapi's default session management and it was making things way more complicated, so I ended up using my own session-library in the example.
If you like it, i can potentially post a PR or something.
It looks really great! Please feel free to create a PR.
PR posted: https://github.com/aliev/aioauth/pull/107
Any
toUserType
to make all generics that use it optional.setup.py
andsetup.cfg
withpyproject.toml
.