Skip to content
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

Typing infrastructure improvements #120

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

Conversation

parfeniukink
Copy link
Contributor

  1. Static type checkers, based on pyright won't work correctly due to the hierarchy of the exported classes and modules (examples from LSP / VSCode pylance presented on screenshots). It means, that for instance smart tools like mypy will definitelly work correctly, but real-time type checkers will distruct developers all the time.
  2. The base classes, or let's say what do you call Generic classes are placed on the same level from the hierarchy perspective which means that it is quite hard to use. According to some sorts of best-practices it is recommended having something like the infrastructure layer for the project that is in development.
    2.1. In this PR you'll find the infrastructure layer that encapsulates all the shared logic for the whole application and make it linear from modules importing perspective (now, there is no imports on the same level)
  3. All the if TYPE_CHECKING: lines are removed due to it's unnecessary. Basically this line might be useful if you have some architecture issues (circular imports) in your code but seems there is no reason to overcomplicate the project with additional if statement in all concrete implementations. Even thought if we decide having them in the project I would recommend using from __future__ import annotations over the if TYPE_CHECKING:
  4. All the docstrings are unified
  5. Imports are simplified with __all__. Since the __all__ could be used to control wild imports in Python this is definitely a good way to go with in my opinion. In the old version of the code the controll of the import is on the __init__.py which make developers to think about a couple of places to export their modules. With current approach you control your concrete SSO implementation in your single file which make the code more robust from changes perspective. Like, all the names that are allowed to be imported from this module are defined in this module itself. And, the way to import this module is defined on the level above (on the package level).

Alacritty 2024-01-28
Code 2024-01-28

So basically this is the result of the typings improvements:

Alacritty 2024-01-28
Code 2024-01-28

Copy link

codecov bot commented Jan 28, 2024

Codecov Report

Merging #120 (be1555c) into master (cb86159) will increase coverage by 2.90%.
The diff coverage is 92.20%.

Additional details and impacted files
@@            Coverage Diff             @@
##           master     #120      +/-   ##
==========================================
+ Coverage   88.16%   91.07%   +2.90%     
==========================================
  Files          14       16       +2     
  Lines         338      336       -2     
==========================================
+ Hits          298      306       +8     
+ Misses         40       30      -10     
Files Coverage Δ
fastapi_sso/__init__.py 100.00% <100.00%> (ø)
fastapi_sso/infrastructure/__init__.py 100.00% <100.00%> (ø)
fastapi_sso/infrastructure/factories/__init__.py 100.00% <100.00%> (ø)
fastapi_sso/infrastructure/factories/provider.py 100.00% <100.00%> (ø)
fastapi_sso/infrastructure/openid.py 100.00% <100.00%> (ø)
fastapi_sso/sso/__init__.py 100.00% <100.00%> (ø)
fastapi_sso/sso/facebook.py 92.30% <100.00%> (+6.59%) ⬆️
fastapi_sso/sso/github.py 56.00% <100.00%> (+2.15%) ⬆️
fastapi_sso/sso/gitlab.py 92.30% <100.00%> (+6.59%) ⬆️
fastapi_sso/sso/kakao.py 92.30% <100.00%> (+6.59%) ⬆️
... and 6 more

fastapi_sso/__init__.py Fixed Show fixed Hide fixed
fastapi_sso/__init__.py Fixed Show fixed Hide fixed
fastapi_sso/sso/__init__.py Fixed Show fixed Hide fixed
@tomasvotava
Copy link
Owner

Hi @parfeniukink, thanks! From the first read I'd say it's very nice, although I am not a huge fan of the infrastructure subpackage name. I'll have another look, preferably tomorrow and either approve or propose some actual changes. Good job!

@parfeniukink
Copy link
Contributor Author

Hi @parfeniukink, thanks! From the first read I'd say it's very nice, although I am not a huge fan of the infrastructure subpackage name. I'll have another look, preferably tomorrow and either approve or propose some actual changes. Good job!

Well, yeah. I agree with the infrastructure naming, but anyway, I just chose the default naming for the shared module right from the DDD approach since there are no other approaches discussed/declared yet. And I hope you agree that at least any of the familiar design approaches in the dev community is better than no one :)

btw, I think that infrastructure layer is kind of a good naming bud not for the 3-rd party package, like fastapi-sso which must be a part of the infrastructure layer in other applications 😄

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