Skip to content

Do not keep @mention people on merge commits #100

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 1 commit into from
Sep 3, 2020

Conversation

tesuji
Copy link
Contributor

@tesuji tesuji commented Sep 1, 2020

I'm not sure it is the right fix or how to test it.
If you could point out, I will give a try.

Closes #99 .

@tesuji tesuji force-pushed the no-mention branch 3 times, most recently from f4e2f07 to cd37cdf Compare September 1, 2020 02:52
@tesuji tesuji marked this pull request as ready for review September 1, 2020 03:02
@tesuji tesuji force-pushed the no-mention branch 2 times, most recently from 5c9f7c7 to 9b16f9e Compare September 1, 2020 05:05
@pietroalbini
Copy link
Member

Homu uses pytest for tests, maybe you could add a test to make sure the suppress_pings function does what we want?

@tesuji
Copy link
Contributor Author

tesuji commented Sep 1, 2020

That doesn't test escape pings from PR body. Till I don't know if it works as intended.
I could add a test for suppress_pings only if you find it is enough.

@pietroalbini
Copy link
Member

Yeah it doesn't test that, but homu doesn't really have a test suite. You could write integration tests for all of it, but like, that's unreasonable to even ask :)

Given the current situation with homu tests, I think just testing that function to ensure it doesn't mess output up is enough.

@tesuji
Copy link
Contributor Author

tesuji commented Sep 1, 2020

Done.


expect = (
"r? `@matklad`\n"
"`@bors` r+\n"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This case is unfortunate, but I assume nobody use @bors r+ in PR body.

@pietroalbini
Copy link
Member

Hmm, are you sure GitHub doesn't ping if in a commit the mention is inside a code block? My understanding is that GitHub completly ignores markdown there.

@tesuji
Copy link
Contributor Author

tesuji commented Sep 3, 2020

let's try @pietroalbini.

@tesuji
Copy link
Contributor Author

tesuji commented Sep 3, 2020

GitHub doesn't ping if in a commit the mention is inside a code block

I don't receive the pings, do you receive one with bfa1814 ?

@pietroalbini
Copy link
Member

Neither did I! TIL

If you remove those two commits we're good to merge this.

@tesuji
Copy link
Contributor Author

tesuji commented Sep 3, 2020

Removed. Thanks for the reviewing!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[feature request] Replace @<name> in PR description with only <name>
2 participants