-
Notifications
You must be signed in to change notification settings - Fork 42
Refactor AdminRouter.__init__
for easier customisation
#442
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
base: master
Are you sure you want to change the base?
Refactor AdminRouter.__init__
for easier customisation
#442
Conversation
@hubert-springbok I think your changes are great. I tried this PR and all the tests passed and everything works great. There are a few linter errors (mostly long docstring and comments), but that's minor problem. Thanks for your effort and time. |
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #442 +/- ##
==========================================
+ Coverage 93.42% 94.49% +1.07%
==========================================
Files 5 5
Lines 365 436 +71
==========================================
+ Hits 341 412 +71
Misses 24 24 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
I've resolved your comments and shortened most lines to 79 characters - let me know if anything else is needed before merge |
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.
Other then some minor typing changes, looks good to me!
Apply suggestions from code review Co-authored-by: Ethan <[email protected]>
@dantownsend this PR is ready to go, just waiting on your approval & merge |
This PR has been marked as stale because it has been open for 30 days with no activity. Are there any blockers, or should this be closed? |
@Skelmis , @dantownsend , are you still looking to merge this one? It seemed to be ready, but some merge conflicts occured since |
My apologies for that, I am still quite keen for this @hubert-springbok. @dantownsend is also keen but is just a tad time limited at the moment to properly dive into the PR. If your all good to resolve the merge conflicts I'll follow back up on our end |
@hubert-springbok Here is the updated |
Thanks @sinisaos , I've pushed that to the branch now |
This PR has been marked as stale because it has been open for 30 days with no activity. Are there any blockers, or should this be closed? |
The refactor intends to keep all the functionality excactly the same and mostly moves blocks of code into helper methods.
However, in the course of the refactor, I made several changes:
superuser_tables
variable to reflect certain usage patterns forauth_table
andsession_table
handle_auth_exception
, which was causing a type errorDespite best efforts, some of the diff is not easy to read, but I hope you'll be able to review and gain confidence in the changes.