-
-
Notifications
You must be signed in to change notification settings - Fork 13
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
PR merge messages need more info #25
Comments
This idea also came from IRC discussions in Sopel's channel. Also, I should have stuck with my gut for which milestone to assign this. I was right the first time. >.< |
I want to get 0.2.0 out relatively soon, and this issue warrants spending enough time to consider all the different possible outputs and develop a unified structure to them all that will allow presenting more details in more places without seeming cluttered or funny-looking. Punted to 0.3.0. 😸 |
So the title of this issue (about PR merge messages) doesn't match the example in the top comment (actually about PR review messages). I'm making some improvements to PR merge messages, but I'm unsure what to do about the review ones, since we can quickly run into over-length output if we include both the PR title and comment on a review event. Thoughts on including the title only if the review comment body is empty? Examples:
Note: I'm really not sure what to do for punctuation around the title. I don't want to put the title after |
title - oops. I think it was part of a "a lot of those messages need work" discussion. I think "pull request" can be safely abbreviated PR.
The github links aren't long, I don't think it would be horrible to put it before the comment if you don't want to truncate like this. |
Hmm, 20 chars is barely longer than some "module.path:" prefixes on the main Sopel repo. That seems awfully short. So does 110 characters of comment. The links won't necessarily be short, e.g. if calling I'm getting an ever-stronger feeling that I should just finish whatever last few things I can for 0.4 and then dive into an almost complete rewrite of the webhook/formatting stuff for 0.5 (or just call it 1.0 because there will be a LOT of changes). We simply need a system that's built with configurability in mind, both for which events should actually generate output and what pieces of info should be included in each message type (plus accommodates long content in a nicer way than just letting the protocol cut things off). Or, if it interests you, feel free to have at it—but maybe start with a general outline before implementing anything. I know I will. |
Let's reassess this and maybe develop a concrete list of improvements once 0.4.0 is out. |
Current message when a PR is merged:
<Sopel> [sopel] dgw approved pull request #1516 https://git.io/fjqGw
This should have at least the name of the user who opened it and the title, like normal PR link info display:
<Sopel> [GitHub] [sopel-irc/sopel #1539] deathbybandaid: Provide a simpler way to kick users from channels. | this adds a `bot.kick(text, channel, nick)` ability to the bot, instead of using `bot.write(['KICK', channel, nick], text)`. ...
It would be good to check other simple action messages, like issue/pr closed, for format consistency or similar missing info.
The text was updated successfully, but these errors were encountered: