Skip to content

Hide old rust-timer comments when a summary is posted #1490

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

Merged
merged 2 commits into from
Nov 4, 2022

Conversation

Kobzol
Copy link
Contributor

@Kobzol Kobzol commented Nov 3, 2022

When a perf. summary is posted to a PR, all other non-summary comments made by the bot on the PR will be minimized. I copied the code from rust-lang/rust-log-analyzer#56 to implement this.

I have no idea how to test this though 🤷‍♂️

Fixes: #1488

@Kobzol Kobzol force-pushed the github-hide-comments branch from 03fa10d to f8545c9 Compare November 4, 2022 13:03
@Kobzol
Copy link
Contributor Author

Kobzol commented Nov 4, 2022

Added a comment about viewer_did_author and changed the marking logic from allowlist to denylist.

Copy link
Member

@rylev rylev left a comment

Choose a reason for hiding this comment

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

Looks good! We can still consider in another PR hiding old perf result summaries if we can figure out how to get that logic right.

@Kobzol
Copy link
Contributor Author

Kobzol commented Nov 4, 2022

I think that old summaries can actually be quite useful, but we can open a discussion about that later I guess.

@Kobzol Kobzol force-pushed the github-hide-comments branch from f8545c9 to 8966336 Compare November 4, 2022 19:01
@Kobzol
Copy link
Contributor Author

Kobzol commented Nov 4, 2022

I suppose that there's not a practical way of testing this without merging this? 😅 @Mark-Simulacrum

@Mark-Simulacrum
Copy link
Member

I would just merge.

@Kobzol Kobzol merged commit bff7ad6 into master Nov 4, 2022
@Kobzol Kobzol deleted the github-hide-comments branch November 4, 2022 20:35
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.

Hide previous perf bot comments
3 participants