-
Notifications
You must be signed in to change notification settings - Fork 0
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
DNM 75 mongo transactions nesbitt #85
Conversation
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.
Just one small thing, otherwise looks good to go.
@@ -61,6 +61,12 @@ app.use( | |||
return; | |||
} | |||
|
|||
if (req.session) { | |||
// TODO: Abort transaction is async |
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.
Rule of thumb I've been using with westegg now since we have multiple people working on it is to not add any more todos but instead just open a ticket for it. If the line is specific and you want to make sure you remember the context then link it in the issue you make.
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.
The PR is still in progress. I left that in there so I would remember to discuss it during our work session today
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.
Next time I'll prefix it with Draft: or something so it doesn't accidentally get merged
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.
Use DNM for (Do no merge) in the PR title. That way it can be for drafts or if something is wrong.
Closing, as out of scope for Pre-Alpha |
Closes #75