github-issue-review.md

Thank you for your code submission and I hope you find this review useful.

#!/usr/bin/env ruby

require 'bundler/setup'
require 'octokit'

1. Improved early error checking on arguments

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 if ARGV.empty? you could use :

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

2. Rescue blocks on client setup

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

3. Manage arguments so that we properly handle issue updates and longer messages

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 an Array that contains the remaining message and potentially the ISSUEID. Lets convert words from an Array to a String so we can continue processing it as a String.

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 the words variable is a Integer or not so that it can be assigned to the issue variable.

The issue variable assignment should then happen in two steps.

1. First determine if the first word in words variable can be converted to an Integer or not, set it to nil if the conversion fails with the rescue statement. We'll use .partition to get the first word from the words variable. If the Integer conversion fails then rescue will catch the error and assign nil as the value.

   issue = Integer(words.partition(' ').first) if Integer(words.partition(' ').first) rescue nil

2. If issue was assigned a number and is not nil, then remove the first word from words 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.

4. Additional considerations for final client calls

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:

1. Consider using the socket library here and the method Socket.gethostname instead of a call out. This may prevent this code from being portable on platforms that don't include the hostname command (like Windows).
2. 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 a nil results from ENV.
3. 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 for hostname -s into system('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