Skip to content
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

Make Github messages more readable #34

Merged
merged 3 commits into from Sep 27, 2016

Conversation

@florasaramago
Copy link
Contributor

@florasaramago florasaramago commented Sep 22, 2016

We've been using this script a lot with Mattermost at the company I work for, and we love it. But since we have many repositories and a lot of daily activity, our code channel was getting a little messy, so I made some changes to the messages to make them more readable. I hope other people might like it as well.

@parkr
Copy link
Member

@parkr parkr commented Sep 22, 2016

This is wonderful, thank you!

Have you run this with your production hubot perchance? We don't have any tests for this (heh) so if you could try it and let me know that it's working for you in all the changed cases, that would be wonderful. 😄

Also, is this syntax a Mattermost-specific syntax? If so, do you know if there's a way to make this platform-agnostic? Slack, Mattermost, and Campfire all have their own link formats.

@parkr parkr added the enhancement label Sep 22, 2016
@florasaramago
Copy link
Contributor Author

@florasaramago florasaramago commented Sep 23, 2016

I'm so glad you like it! 😄

Yes, I made these changes in our production hubot first, and we have been using it like this for about a week. So far we didn't have any problems.

About the syntax, that's a good point. I checked Slack's documentation and apparently they use the same link format as Mattermost:
https://api.slack.com/docs/message-formatting#linking_to_urls

But I can't find any documentation on how Campfire or HipChat handle links (let alone all the other platforms), so I'm guessing they probably don't have this option. I think one possibility would be to have a separate class that would receive the data object and return the formatted message according to the current adapter. What do you think?

@parkr
Copy link
Member

@parkr parkr commented Sep 23, 2016

I think one possibility would be to have a separate class that would receive the data object and return the formatted message according to the current adapter. What do you think?

I think that sounds great! I believe robot.adapter provides the adapter name. If Mattermost and slack use the same formatting, then it should be a quick fix to create the new class or function that doesn't check the value of robot.adapter (but receives it) and returns a formatted URL based on the input. Then if folks using HipChat or Campfire upgrade, they could update the function to format properly for them and I can make a new release then. How does that sound? So just pulling the formatting out into a function or class then dealing with other adapters later.

@florasaramago
Copy link
Contributor Author

@florasaramago florasaramago commented Sep 26, 2016

Done! I'm not sure I did it in the best possible way, because I'm just starting to get into this node/coffee/hubot world (I'm originally a Rails developer), so let me know if there's a better way to accomplish this. :)

But in any case, it works! The links are still showing nicely in Mattermost and I tested it the other way around too (displaying only the URL when the adapter is Mattermost), and it also works. Now it's just a matter of adding "whens" to the switch and it should work for any adapter!

Copy link
Member

@parkr parkr left a comment

Thank you so much! Just one nit about escaping the hash symbol and a comment about the order of parameters. Code looks solid though! 👍

issue = data.issue
comment = data.comment
repo = data.repository
repo_link = formatUrl adapter, repo.html_url, repo.name
comment_link = formatUrl adapter, comment.html_url, "#{issue_pull} \##{issue.number}"

This comment has been minimized.

@parkr

parkr Sep 27, 2016
Member

I don't believe you have to escape the # symbol in CoffeeScript. The code you have here, per coffeescript.org, produces:

issue_pull + " \#" + issue.number;
pull_num = data.number
pull_req = data.pull_request
base = data.base
repo = data.repository
repo_link = formatUrl adapter, repo.html_url, repo.name
pull_request_link = formatUrl adapter, pull_req.html_url, "\##{data.number} \"#{pull_req.title}\""

This comment has been minimized.

@parkr

parkr Sep 27, 2016
Member

Don't need to escape the first # here either.

if eventActions[eventType]?
eventActions[eventType](data, cb)
eventActions[eventType](data, cb, adapter)

This comment has been minimized.

@parkr

parkr Sep 27, 2016
Member

I'm OK with this though it's usually customary (from what I remember) for the callback (cb here) to be the last parameter. That is kind of annoying to go back and change so if you don't want to do it, no sweat. 😄

@florasaramago
Copy link
Contributor Author

@florasaramago florasaramago commented Sep 27, 2016

Done! I fixed the order of the parameters too, it was no trouble at all 😄

@parkr
Copy link
Member

@parkr parkr commented Sep 27, 2016

This is GREAT! 🎉 💖

Thank you so much for your contribution and for sticking with me through my reviews. I'll merge and release this now!

@parkr parkr merged commit 1c9a48b into hubot-scripts:master Sep 27, 2016
@parkr parkr mentioned this pull request Sep 27, 2016
@parkr
Copy link
Member

@parkr parkr commented Sep 27, 2016

Looks like I can't release to NPM. For now, you can add this to your package.json:

"hubot-github-repo-event-notifier": "https://github.com/hubot-scripts/hubot-github-repo-event-notifier.git#v1.7.0"

I think that should work. :)

@florasaramago
Copy link
Contributor Author

@florasaramago florasaramago commented Sep 27, 2016

Awesome!!!!! 🎉 🎉 🎉

Thank you too, for taking the time to review my code! I had a lot of fun with this. I'm getting addicted to Hubot, so I hope to make more contributions soon!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.