-
Notifications
You must be signed in to change notification settings - Fork 109
[sql-20] sessions: more re-shaping #993
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
Conversation
Rename it to GetSessionByLocalPub so that it is more accurately named and so that we free up the GetSession name for future use.
By default, we fetch records by an ID.
It's a better pattern to refer to sessions in the same way consistently. So we update the UpdateSessionRemotePubKey method to use a session ID as a reference to the session instead of local pub key.
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.
LGTM 🚀!
| session, err := DeserializeSession( | ||
| bytes.NewReader(serialisedSession), | ||
| ) | ||
| session, err := getSessionByID(sessionBucket, id) |
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.
Nit: Maybe rename the internal getSessionByID to getSession as well :)?
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.
this is verryyyy nitty yall 😅 i think exposed/exported methods are the main focus. especially since we will delete all this code kvdb code by the end of the series
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.
Nice 🎉
Ok actual actual last moulding PR before plugging in the SQL backend :)
See commit messages for more details.