Skip to content

Cherry pick PR #3205 into release/v1.0 branch.#4143

Merged
martinmr merged 1 commit intorelease/v1.0from
martinmr/cp-expand-all-fix
Oct 14, 2019
Merged

Cherry pick PR #3205 into release/v1.0 branch.#4143
martinmr merged 1 commit intorelease/v1.0from
martinmr/cp-expand-all-fix

Conversation

@martinmr
Copy link
Copy Markdown
Contributor

@martinmr martinmr commented Oct 8, 2019

This fixes #3807.


This change is Reviewable

Copy link
Copy Markdown

@hackerone-code hackerone-code Bot left a comment

Choose a reason for hiding this comment

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

✅ A review job has been created and sent to the PullRequest network.


@martinmr you can click here to see the review status or cancel the code review job.

@martinmr martinmr requested review from a team, danielmai and manishrjain October 8, 2019 21:45
Copy link
Copy Markdown

@hackerone-code hackerone-code Bot left a comment

Choose a reason for hiding this comment

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

LGTM with some nits on the comments.


Reviewed with ❤️ by PullRequest

Comment thread query/query.go
}

if sg.DestUIDs == nil || len(sg.DestUIDs.Uids) == 0 {
// Looks like we're done here. Be careful with nil srcUIDs!
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

We should add more context to this comment. Why should we be careful with srcUIDs == nil? We don't use it here either.

Comment thread query/query.go
}
out = append(out, child)
}
sg.Children = out // Remove any expand nodes we might have added.
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Comment should be on top of the for loop, since its describing the for loop process.

Copy link
Copy Markdown
Contributor

@manishrjain manishrjain left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 3 of 3 files at r1.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @danielmai and @martinmr)

@martinmr martinmr merged commit 8299915 into release/v1.0 Oct 14, 2019
@martinmr martinmr deleted the martinmr/cp-expand-all-fix branch October 14, 2019 17:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants