-
Notifications
You must be signed in to change notification settings - Fork 134
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
Implemented user sessions #405
base: master
Are you sure you want to change the base?
Conversation
@tusharad Thank you! I think this is a very good PR. Just couple of minor remarks on documentation and a return type. |
Hi @tusharad , hope you're doing well! I was wondering whether you need any help to land this PR? Please let us know :) |
Hi @ocramz, I believe there might have been a misunderstanding. Since you self-assigned this task, I assumed you would be implementing some of the changes yourself.
Could you please elaborate on the specific return type and areas in the documentation you’d like to see revised? This will help me address your feedback more effectively. Thanks! |
Hi @tusharad , yes apologies about that! The return type I was mentioning is that of |
Hi @ocramz, Thank you for the feedback! In the latest update, I’ve replaced Nothing with Either and introduced a new type called SessionStatus, which can represent SessionExpired or SessionNotFound. Please let me know if there’s anything else you’d like to adjust or further refine. I appreciate your input! |
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.
A few outstanding comments, but mainly:
- missing ActionM specialized Session functions
- changelog entry with issue number
- Session module header with correct authors and copyright
Optional but nice to have :
- add a mention of the new Session functionality in the README
Web/Scotty/Session.hs
Outdated
threadDelay 1000000 | ||
maintainSessions sessionJar | ||
|
||
-- | Adds a new session to the session jar. |
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.
Adds or overwrites, that is
changelog.md
Outdated
@@ -1,5 +1,6 @@ | |||
## next [????.??.??] | |||
|
|||
* Added sessions. |
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.
Could you please refer to the github issue number here?
Web/Scotty.hs
Outdated
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.
As far as I can see, this file (Web.Scotty) is missing the specialized versions of the Session
functions, returning in the ActionM
monad. See e.g. https://hackage.haskell.org/package/scotty-0.22/docs/src/Web.Scotty.html#request
Thanks for the review @ocramz!
Do let me know in anything else is required. Thanks again! |
@tusharad Thanks for the revision!
Yes please, open a new ticket for this. |
@tusharad thanks! There's still time to add a line to the README, in the Examples section :) |
This PR introduces user session management for the Scotty, addressing the requirements outlined in issue #317. The implementation draws inspiration from the Scotty Session library.
Checklist:
Dependency Update:
Added the
random
package to support the generation of session IDs. Please let me know if this addition is acceptable.Do let me know if there are any further changes or additions you need. Thank you for reviewing this PR!
@ocramz