-
-
Notifications
You must be signed in to change notification settings - Fork 7
Conversation
90b901b
to
a1a2f03
Compare
lib/helpers/tasks.rb
Outdated
else | ||
slack_channel = '#default' | ||
end | ||
slack_channel = if settings.slack_channel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't this be slack_channel = settings.slack_channel || '#default'
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, it cannot. At least not yet.
The way Sinatra::ConfigFile
works is that it will fail with a no method error if the config key doesn't exist in the yml file. My current workaround (which would allow this to work) is to have an empty key in the yml config file so that it will return nil.
I have some work to do on the config file front.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, nevermind. The NoMethodError
would be triggered either way if the setting is missing. So, I agree with @ekohl, let's simplify that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general I like having a config file which has all the defaults in it rather than the code. It's a nice reference for users and can simplify code like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comment thread on slack_channel.settings
lib/helpers/tasks.rb
Outdated
else | ||
slack_channel = '#default' | ||
end | ||
slack_channel = if settings.slack_channel |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, nevermind. The NoMethodError
would be triggered either way if the setting is missing. So, I agree with @ekohl, let's simplify that.
Modules and classes should have at least one line of documentation.
We can probably remove this later after we've done more refactoring.
Not much point in fixing these violations here. The code should be moved to the new parser class.
a1a2f03
to
b5ebee0
Compare
The commits are mostly the result of doing individual auto-corrects (and doing a quick review for correctness). I also made sure that each new commit didn't undo any of the cops that were already fixed. From Style/IfInsideElse on, there was nothing left that could be autocorrected and those are manual work. |
Partially addresses #3 |
No description provided.