Skip to content
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

Add heroes to the room summary #2373

Merged
merged 7 commits into from
Apr 26, 2022
Merged

Add heroes to the room summary #2373

merged 7 commits into from
Apr 26, 2022

Conversation

S7evinK
Copy link
Contributor

@S7evinK S7evinK commented Apr 25, 2022

Needs matrix-org/sytest#1246, as otherwise the tests will flake.

@S7evinK S7evinK requested a review from a team as a code owner April 25, 2022 17:43
Copy link
Contributor

@neilalexander neilalexander left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple things, otherwise LGTM.

ctx context.Context, txn *sql.Tx, roomID, userID string, memberships []string,
) (heroes []string, err error) {
stmtSQL := strings.Replace(selectHeroesSQL, "($3)", sqlutil.QueryVariadicOffset(len(memberships), 2), 1)
stmt, err := s.db.Prepare(stmtSQL)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You'll need to close this too, otherwise the prepared statements leak.

type membershipsStatements struct {
db *sql.DB
upsertMembershipStmt *sql.Stmt
selectMembershipCountStmt *sql.Stmt
selectHeroesStmt *sql.Stmt
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we have to prepare to do variadics, there isn't much point in preparing it up front. Normally I just leave these commented out so that it's obvious there's something but we don't waste the effort preparing a statement we never use.

@S7evinK S7evinK merged commit e8be2b2 into main Apr 26, 2022
@S7evinK S7evinK deleted the s7evink/roomsummary branch April 26, 2022 08:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants