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

Simplify #175

Merged
merged 4 commits into from
Oct 15, 2023
Merged

Simplify #175

merged 4 commits into from
Oct 15, 2023

Conversation

blag
Copy link
Contributor

@blag blag commented Oct 15, 2023

Using some of the code from Django, we can greatly simplify and even remove some of our code.

Description

  • I adjusted our middleware to inherit from Django's so we don't have to duplicate any of that code
    • Django imports the session engine only once, instead of on each request - this also simplifies our module imports
    • We don't have to define our own process_response at all, since it wasn't materially different from Django's
  • I tweaked our session backend to inherit from Django's, and tweaked it to use some of the methods defined in the superclass

Motivation and Context

We duplicated a lot of code from django.contrib.sessions. The less code we duplicate, the less code we have to maintain ourselves.

This might also make it easier to add a cached_db backend similar to Django's.

How Has This Been Tested?

Normal tests.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Reverse-compatible code improvement

Checklist:

  • My code follows the code style of this project.
  • All new and existing tests passed.

@codecov
Copy link

codecov bot commented Oct 15, 2023

Codecov Report

Merging #175 (ef28ea0) into master (5ab2c57) will increase coverage by 1.92%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #175      +/-   ##
==========================================
+ Coverage   94.92%   96.84%   +1.92%     
==========================================
  Files          16       16              
  Lines         768      698      -70     
  Branches       65       56       -9     
==========================================
- Hits          729      676      -53     
+ Misses         29       16      -13     
+ Partials       10        6       -4     
Files Coverage Δ
user_sessions/backends/db.py 100.00% <100.00%> (+8.82%) ⬆️
user_sessions/middleware.py 100.00% <100.00%> (+29.72%) ⬆️

... and 2 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Member

@WhyNotHugo WhyNotHugo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have some questions of some bits that are not clear.

4527600 looks good, I can go ahead and merge that single commit in the meantime.

user_sessions/backends/db.py Show resolved Hide resolved
user_sessions/middleware.py Show resolved Hide resolved
@WhyNotHugo
Copy link
Member

Oh, darn, I can't merge 4527600 since merging into this repository is only allowed by creating a separate PR.

I'll merge them all together when all patches have are ready.

@blag
Copy link
Contributor Author

blag commented Oct 15, 2023

I have responded to your questions. If I have done so to your satisfaction, please resolve them. Otherwise I'm happy to answer additional questions or explain what's going on in a bit more detail. The interplay in the middleware and backends between Django's superclasses and ours can be a little complicated, so please take your time and ask for whatever explanations you need.

I realize that this PR is quite a big change and while I'd like to see it merged, I'm trying very hard to not be pushy about it. 😄

There is an argument to be made that our previous code was much more easily understandable than the current code, even though it duplicated a lot of code from Django. However, since it doesn't seem like this project has tons of attention paid to it, reducing the amount of code we have to maintain in this project and relying on subclassing Django objects is a worthwhile tradeoff in my opinion, even if it does make the code slightly more difficult to figure out what it's doing.

@blag
Copy link
Contributor Author

blag commented Oct 15, 2023

I added a few more comments to give some pointers as to where certain methods are used by methods on the superclass. Happy to add more if they'll be useful or helpful.

user_sessions/backends/db.py Show resolved Hide resolved
user_sessions/middleware.py Show resolved Hide resolved
@blag blag merged commit 8064ec8 into master Oct 15, 2023
38 checks passed
@blag blag deleted the simplify branch October 15, 2023 20:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants