Skip to content

Conversation

hubert-springbok
Copy link

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:

  • add a bunch of private attributes to facilitate access to the constructor data from the helpers
  • add SessionExpiryConfig dataclass to group arguments around this topic
  • renamed private_app to api_app to reflect endpoint naming
  • introduce superuser_tables variable to reflect certain usage patterns for auth_table and session_table
  • remove type hint on handle_auth_exception, which was causing a type error

Despite 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.

@sinisaos
Copy link
Member

sinisaos commented Apr 4, 2025

@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-commenter
Copy link

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

Attention: Patch coverage is 97.16981% with 3 lines in your changes missing coverage. Please review.

Project coverage is 94.49%. Comparing base (1157c12) to head (2d2f8a2).
Report is 36 commits behind head on master.

Files with missing lines Patch % Lines
piccolo_admin/endpoints.py 97.16% 3 Missing ⚠️

❗ 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.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@hubert-springbok
Copy link
Author

I've resolved your comments and shortened most lines to 79 characters - let me know if anything else is needed before merge

Copy link
Contributor

@Skelmis Skelmis left a 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]>
@hubert-springbok hubert-springbok requested a review from Skelmis April 8, 2025 07:56
@Skelmis
Copy link
Contributor

Skelmis commented May 7, 2025

@dantownsend this PR is ready to go, just waiting on your approval & merge

Copy link

github-actions bot commented Jun 7, 2025

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?

@github-actions github-actions bot added the Stale label Jun 7, 2025
@hubert-springbok
Copy link
Author

@Skelmis , @dantownsend , are you still looking to merge this one? It seemed to be ready, but some merge conflicts occured since

@Skelmis
Copy link
Contributor

Skelmis commented Jun 9, 2025

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

@github-actions github-actions bot removed the Stale label Jun 10, 2025
@sinisaos
Copy link
Member

@hubert-springbok Here is the updated endpoints.py file from your PR. This change should resolve PR merge conflicts. Feel free to use it and I hope it helps.

@hubert-springbok
Copy link
Author

Thanks @sinisaos , I've pushed that to the branch now

Copy link

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?

@github-actions github-actions bot added the Stale label Jul 16, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants