change(notification): simplify (?) the formatting of the reminder #27
Conversation
lib/review_bot/notification.rb
Outdated
| [ | ||
| %("#{pull_request.title}" needs a #{needed_review_type} from), | ||
| %(• <#{pull_request.html_url}|#{pull_request.title}> needs a *#{needed_review_type}* from), |
There was a problem hiding this comment.
| @@ -4,7 +4,7 @@ require 'json' | |||
| CONFIG = JSON.parse(ENV['CONFIG']) | |||
| SLACK_TOKEN = ENV['SLACK_TOKEN'] | |||
| SLACK_BOT_NAME = 'reviewbot' | |||
| SLACK_BOT_ICON = ':smile_cat:' | |||
| SLACK_BOT_ICON = ':robot_face:' | |||
There was a problem hiding this comment.
I really like what you've done here. But I absolutely cannot have this go to production. We need a full basecamp discussion about the correct emoji for this bot
There was a problem hiding this comment.
I think @geolessel's suggestion about including the issue number is good.
Thanks for putting this together
Gemfile
Outdated
| @@ -1,9 +1,15 @@ | |||
| # frozen_string_literal: true | |||
| source 'https://rubygems.org' | |||
|
|
|||
| gem 'awesome_print' | |||
| ruby '2.3.0' | |||
There was a problem hiding this comment.
Since we're doing this, should we remove the .ruby-version?
There was a problem hiding this comment.
¯\_(ツ)_/¯ i want to say that .ruby-version still informs rbenv on which version to use for local development. But maybe that doesn't matter? Except actually it might because there is stuff that's only available in 2.3+? Except maybe the right answer is to just check for positivity the old fashioned way?
There was a problem hiding this comment.
I'm happy with any of those. I'm just gonna drop .positive? in master
There was a problem hiding this comment.
Ok, master no longer uses positive?
shanebonham
force-pushed
the
sb/notification-format
branch
2 times, most recently
from
acfc316 to
836ae6e
Compare
shanebonham
force-pushed
the
sb/notification-format
branch
from
836ae6e to
2726162
Compare
|
@danielma am I supposed to be able to merge this? I don't see a merge button. |
|
Oh. I had to accept the invitation first. |
I thought this might clean things up a bit, sort of in response to #22
before
after