updated with fixes from flask-session2 #170
Conversation
Codecov Report
❗ Your organization is not using the GitHub App Integration. As a result you may experience degraded service beginning May 15th. Please install the Github App Integration for your organization. Read more. @@ Coverage Diff @@
## main #170 +/- ##
=======================================
Coverage ? 67.07%
=======================================
Files ? 3
Lines ? 1063
Branches ? 0
=======================================
Hits ? 713
Misses ? 350
Partials ? 0 📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
|
Some of the tests randomly fail on I'm thinking to get around the error we could run the tests in a specified order (file first, then memcached) Its odd - you can see the same tests running here and never failing: https://github.com/christopherpickering/flask-session2/actions The difference is that these tests are not running in poetry and the others are. There must be some other level of issolation between tests in repo2. The other thing I see is another werkzueg warning.. also flask-session never closes out connections? I wonder if there is a way to properly do that? |
|
This is awesome PR While this is merged |
| config.setdefault('SESSION_SQLALCHEMY_TABLE', 'sessions') | ||
|
|
||
| if config['SESSION_TYPE'] == 'redis': | ||
| config.setdefault("SESSION_TYPE", "null") |
There was a problem hiding this comment.
Thanks for making the first cut, @christopherpickering ! This PR used to have more than 10K lines, now it reduces to 1.5K.
But it is still somewhat large. From a tactical standpoint, I would suggest to create another PR with styling changes only, which would be much straightforward to review and merge. And then we can rebase this PR which will then be much smaller. You do not have to create that style PR, I can do that for you/us, if you prefer. Just let me know. By the way, what linter do you use?
There was a problem hiding this comment.
I don't have time to spend on the project, I don't even use flask sessions anymore :)
I normally use black with pre-commit. I think this code hasn't been passed through a formatter yet?
Thanks!
|
|
||
| with app.app_context(): | ||
| if not inspect(db.engine).has_table("Session"): | ||
| self.db.create_all() |
There was a problem hiding this comment.
Hmm why to call create_all here? This will affect all tables instead of just the needed one, which will be very confusing
There was a problem hiding this comment.
Something like this should not be there at all! Any developer who uses alembic to maintain DB creation etc will be VERY confused and eventually pissed when finding out that some library executes DDL on its own.
|
Having |
|
The PR title states that this merges in changes from flask-session2 but that was done in #160, is the PR title wrong? With all that said, can this be closed out with individual PRs where applicable and keep them tight in scope? Additionally in the PR I created #179, I updated the testing to use pytest, this will show us the code coverage during github workflows execution. Ideally, we would use the standard testing layout defined there. |
|
Closing this as have now incorporated effectively all of the fixes. Some backends have not yet been added but will eventually be. Regarding create_all, I will open a new issue directly about this. |
This pr merges the updates from flask-session2 and should close out the following issues:
#134
#128
#44
#36
#29
#26
#11
#46
#66
#53
#56
#85
#89
#67
#150
#158
#157
#137
#151
#131
#75
#121
#113
#63
hopefully #79
This overlaps/clones the following pr's
#12
#32
#34
#57
#80
#90
#93
#96
#100
#102
#105
#109
#117
#135
#155
This pr also includes most fixes from the other community forks. The readme is updated with a list of contributors to those changes.