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

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 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.

@florasaramago
Copy link
Contributor Author

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 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

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

Choose a reason for hiding this comment

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

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}"
Copy link
Member

Choose a reason for hiding this comment

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

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}\""
Copy link
Member

Choose a reason for hiding this comment

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

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

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

Choose a reason for hiding this comment

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

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

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

@parkr
Copy link
Member

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-archive:master Sep 27, 2016
@parkr parkr mentioned this pull request Sep 27, 2016
@parkr
Copy link
Member

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

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
Development

Successfully merging this pull request may close these issues.

None yet

2 participants