Thank you for your code submission and I hope you find this review useful.
#!/usr/bin/env ruby
require 'bundler/setup'
require 'octokit'
Let's improve argument handling so we can catch if no message or issue is passed. It's possible that we could have a repo and nothing else, but the argument handling does not deal with the case for when one argument is passed (the repo). It doesn't make much sense to create an empty issue with no message.
Instead of
you could use :if ARGV.empty?
if ARGV.empty? or ARGV.length < 2
if ARGV.empty?
puts "Post new issues or, if given an issue id, add a comment."
puts "Usage: gh-simple-issue REPO [ISSUEID] <text>"
exit 1
end
Consider adding a
begin
,rescue
block for error handling statement here so that you can catch if authentication fails, such as the token not being set or login not being valid.
token = ENV['HUBOT_GITHUB_TOKEN']
client = Octokit::Client.new(
:login => "hubot",
:access_token => token
)
repo = issue = nil
Now lets improve how we can handle deciding if the second argument is an ISSUEID or part of the message that goes into our issue. For this, we'll need to test if the second argument is a number or simply a string text of the message. It might be better if we handle that with an early assignment such as:
repo, *words = ARGV
This also takes care of the missing message from the third argument and beyond. Also, although
repo
is now a string,words
is now anArray
that contains the remaining message and potentially the ISSUEID. Lets convertwords
from anArray
to aString
so we can continue processing it as aString
.
words = words.join(' ')
Now that we have the
repo
value assigned, it's a fairly safe assumption that we will have at least the remaining message because of the earlier check for more than two arguments for the input. We now simply need to determine if the first word in thewords
variable is aInteger
or not so that it can be assigned to theissue
variable.
The
issue
variable assignment should then happen in two steps.
- First determine if the first word in
words
variable can be converted to anInteger
or not, set it tonil
if the conversion fails with therescue
statement. We'll use.partition
to get the first word from thewords
variable. If theInteger
conversion fails thenrescue
will catch the error and assignnil
as the value.issue = Integer(words.partition(' ').first) if Integer(words.partition(' ').first) rescue nil
- If issue was assigned a number and is not
nil
, then remove the first word fromwords
string and continue.words = words.split(' ')[1..-1].join(' ') unless issue.nil?
** Now you should be ready to remove this section if you choose to implement the new argument handling **
if ARGV.length >= 3
repo = ARGV[0]
issue = ARGV[1]
words = ARGV[2]
elsif ARGV.length >= 2
repo = ARGV[0]
words = ARGV[1]
end
Note: one last possibility in argument handling is that the ISSUEID was provided but no remaining message was given. In this case an empty comment will be appended to the issue, so it might be good to check for that if thats not a desired behavior.
nwo = "github/#{repo.downcase}"
The next two calls for client should have
rescue
blocks to deal with error handling.
if issue
client.add_comment(nwo, issue, words)
else
A few considerations on the next line:
- Consider using the
socket
library here and the methodSocket.gethostname
instead of a call out. This may prevent this code from being portable on platforms that don't include thehostname
command (like Windows).- Consider providing a default for
CAMPFIRE_USER
if the environment variable is not set. This can be done as follows:#{(ENV["CAMPFIRE_USER"] || 'anonymous').strip}
, otherwise the.strip
command will fail on anil
results fromENV
.- This doesn't seem to be an issue at the moment but if in the future you change the
hostname -s
callout to include any variables, you might make this code susceptible to command injection. An early best practice might be to make the call out forhostname -s
intosystem('hostname','-s')
, so that any future edits might not accidentally lead to command injection. Another good reason to consider the first point.
client.create_issue(nwo, "Opened from #{`hostname -s`.strip} by #{ENV["CAMPFIRE_USER"].strip}", words)
end
Thanks! and I hope you enjoyed this review.
-- wenlock