Trim comment, review, etc. bodies at the first <hr>.

[pkaminski]

Whenever converting HTML to mrkdwn, trim the HTML at the first encountered <hr>. Since people typically separate a summary from the details with an <hr> in issues / PRs / reviews, this avoids sending unwanted details to Slack and keeps the notification compact.

Implements #633. Requires integrations/html-to-mrkdwn#25 to be merged and published before merging.

Note that I don't have a Mac so I wasn't able to set up the development environment and run the tests — sorry. Unfortunately Travis won't work either until integrations/html-to-mrkdwn#25 gets merged so I hope I didn't mess up too badly.

[bkeepers]

@pkaminski thanks for opening this pull request!

We discussed this feature as a team, and we don't feel like use of --- is common enough usage for us to build special functionality around. For other users it could lead to confusion about why the full body isn't showing up.

One thing we've considered is adding per-channel or per-subscription configuration options for changing the rendering of messages. For example, a user could run /github set format:concise to make all messages in the channel use a more concise rendering.

What "concise" means is something that warrants some discussion. I could it being a truncated version (at <hr> or even <p> breaks), or just skipping the message body entirely.

Let us know if you're interested in working on that.

[pkaminski]

Thanks for taking a look @bkeepers! I'm definitely interested in resolving this problem, no matter the solution we eventually agree on.

However, first I'd like to push back a bit on the points raised by your team. I agree that --- isn't terribly common, but the code needed to trim after it is pretty small and simple (i.e, has a commensurately low maintenance load) and the logic wouldn't affect anyone who's not using ---. To help ease any confusion we could trim after the first --- and render it as ✂ ✂ ✂ or ✄ ✄ ✄ instead of * * * for example, or as * * * truncated * * * to be more explicit.

My concern with the proposed format:concise setting is lack of discoverability. And unless the definition of "concise" ends up being different than "truncating at ---" it seems preferable to me to just do the obvious, fairly intuitive thing instead of forcing users to jump through an extra hoop. Maybe that's just my bias against overly configurable systems, though. 😄

Perhaps a middle ground could be to introduce a new format:full flag instead, default everyone to concise rendering, and render the first --- as * * * truncated, set format:full to show * * *. This would address both the reasonable default and discoverability concerns.

Thoughts?

[mrmartineau]

@bkeepers a concise option would really be helpful to reduce the "noise" that some messages produce. Any news on that?

[glennpratt]

I'm in favor of the --- option. I find it pretty intuitive that truncation must happen somewhere and to click the link for more. If I want to know how to control exactly where I can read the docs.

We use templates across many repos and they are integrated in many Slack rooms. If we can update them all once to avoid dropping the boilerplate into Slack, that's a big win over configuration.

[n1zyy]

I agree that <hr> isn't super common, but would definitely support something. The team I'm on has a default template for PRs for merge steps with checkboxes, and having it dumped into our Slack channel on every new PR is very noisy. I'd happily move our templates to --- if that's what we split on, but I think splitting on <p> would work well, too.

[ccahoon]

@bkeepers We would also be interested in a concise option. Since your team is already discussing it, I didn't want to presume and make a new Issue for it. But would it be worth doing so, so people can 👍 it if they like?

[pkaminski]

FYI, you can 👍 in issue #633. Thanks for your interest everyone -- I hope this gets GitHub's attention!

[stale]

Is this still relevant? If so, just comment with any updates and we'll leave it open. Otherwise, if there is no further activity, it will be closed.

[pkaminski]

I'm still hopeful this will be considered for merging in at some point.

[tebriel]

Thanks for this PR. Let me reach out to our design team and see how they feel about this change!

[tebriel]

Thanks again for this PR. After some discussion, we're concerned about making this change globally, as the split on <hr> or <p> is somewhat arbitrary for all comments. The discussion likened it to "what if we sent a partial email that split on ---, is that what the user would expect/want?

I like the suggestion from @bkeepers of adding a concise setting, which then could pick something to split on, though it's unclear what it should really be splitting on. What about after the first heading? Or first set of double-newlines? It's hard to pick something that's clear to the user what the behavior will be (and why their message was truncated if they weren't expecting it). Do you have some thoughts on how we could possibly make this configurable, but also with an expected behavior?

[pkaminski]

I don't think it's fair to compare a Slack notification to an email message, as the expectations for content size are very different in these media. Also, Slack notifications are already being truncated at an arbitrary point, so a tweak to where the truncation happens wouldn't be unexpected. Especially so if we add a "Show more" link at the truncation point to mimic what Slack does when it auto-truncates.

I think there's a lot of value in having a standard truncation demarcator in Markdown and in having truncation applied by default even if it's configurable. This will allow GitHub apps to take advantage of this feature without needing to be configured with a custom separator option or asking their users to reconfigure the Slack channel.

I'm not too committed to a specific separator though I think --- is a decent candidate that balances being intuitive with not being too commonly used, and also being fairly reverse-discoverable. ("Huh, why'd this message get cut off here in Slack? Let me check the original... Oh, there's a --- right there, that makes sense. Cool!" This wouldn't be as obvious with a double newline or a heading, I think.)

That said, if you think a flag is necessary, then either my format:full proposal above or @bkeepers' format:concise ought to work. I don't believe making the separator itself configurable is desirable.

[tebriel]

I think adding the format:concise is the right option as it doesn't change the current behavior, makes it opt-in. Let me propose this change (adding the settings) before you start to make sure it's something the team is ok with accepting. Thanks for all the thought you've obviously put into this (and being patient!).

[pkaminski]

Cool, let me know. And thanks for re-engaging on this very old PR!

[stale]

Is this still relevant? If so, just comment with any updates and we'll leave it open. Otherwise, if there is no further activity, it will be closed.

[pkaminski]

Might as well keep this open as long as we're keeping #633 alive...

[tebriel]

/cc @integrations/chatops for visibility into this community issue

[ashokirla]

Hi, Thanks for your contribution. We are going through platform changes to our ChatOps app and are not accepting any external contributions. 
We will go through your PR and consider your suggestions if it aligns with our Product direction. We are converting your PR to a feature request and it can be tracked here.