pallets-eco / flask-session

Server side session extension for Flask
https://flask-session.readthedocs.io
BSD 3-Clause "New" or "Revised" License
490 stars 236 forks source link

updated with fixes from flask-session2 #170

Closed christopherpickering closed 4 months ago

christopherpickering commented 1 year ago

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.

codecov-commenter commented 1 year ago

Codecov Report

:exclamation: No coverage uploaded for pull request base (main@3bb2c6e). Click here to learn what that means. The diff coverage is n/a.

:exclamation: 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           

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

christopherpickering commented 1 year ago

Some of the tests randomly fail on test_filesysestem_session (only on py39?!), but on a key missmatch error w/ memcached. It looks like cachelib.file is using memecached as a backend, if memcached is installed.

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?

idoshr commented 1 year ago

This is awesome PR Please approve it ASAP

While this is merged My PR should be declined https://github.com/pallets-eco/flask-session/pull/155

ThiefMaster commented 1 year ago

Having black reformatting in this PR makes it basically impossible for anyone to properly review... widespread formatting changes do not belong in PRs that add features or bugfixes.

darless commented 1 year ago

The PR title states that this merges in changes from flask-session2 but that was done in #160, is the PR title wrong? I agree with what others stated about this PR having style changes that makes it hard to review, that should be separate. Also workflow changes should be separate, etc.

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.

Lxstr commented 4 months ago

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.