-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Notifications - Step 1 #429
Conversation
NotificationStatusRead | ||
) | ||
|
||
const ( |
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 put the constants in a single block
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.
It's necessary for iota and different types, as discussed below.
UserID int64 `xorm:"INDEX NOT NULL"` | ||
RepoID int64 `xorm:"INDEX NOT NULL"` | ||
|
||
Status NotificationStatus `xorm:"SMALLINT INDEX NOT NULL"` |
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.
Really a separate type for simple integers?
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.
I really like this. :)
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.
If the type got additional functions it makes sense to me, but not for the current status
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.
I also prefer enum, and others too, since I was asked to use this in the previous PR.
Generally not bad, but you should select more collections from the db instead of firing lots of small queries |
Updated. No more queries inside the loop. |
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.
Except that then LGTM
if err := sess.Begin(); err != nil { | ||
return err | ||
} | ||
defer sess.Close() |
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.
put sess.Close()
before sess.Begin()
Deprecated by #523 |
First step to resolve #145
This deprecates #321
I plan 2 steps: