Join GitHub today
GitHub is home to over 28 million developers working together to host and review code, manage projects, and build software together.
Sign upAdd an option for file/STDIN to allow for third party chart data #31
Conversation
Stantheman
added some commits
Sep 1, 2014
akerl
reviewed
Sep 1, 2014
| @@ -9,8 +9,37 @@ OptionParser.new do |opts| | ||
| "Usage: githubchart (-u username) (-t type) path/for/new/image\n" | ||
| opts.banner << 'Supported types: ' + GithubChart.supported.join(' ') | ||
| opts.on('-uUSER', '--user=USER', 'Specify GitHub user to graph') do |user| | ||
| if options.has_key?(:input) |
This comment has been minimized.
This comment has been minimized.
akerl
Sep 1, 2014
Owner
This should probably be "options.include? :input". has_key works, but include is the more conventional way for testing inclusion.
This comment has been minimized.
This comment has been minimized.
coveralls
commented
Sep 1, 2014
akerl
reviewed
Sep 1, 2014
| exit 1 | ||
| end | ||
| unless (input.eql? '-') || (File.exists?(input)) |
This comment has been minimized.
This comment has been minimized.
akerl
Sep 1, 2014
Owner
You may want to check "If file exists or (input equals '-' and stdin isn't a TTY", for sanity
akerl
reviewed
Sep 1, 2014
| exit 1 | ||
| end | ||
| if input.eql? '-' |
This comment has been minimized.
This comment has been minimized.
akerl
Sep 1, 2014
Owner
You may want to make this and the preceding check one larger block. You can also use "fail "string"" to do the error message and exiting, so you could do something like:
if input.eql? '-'
fail 'No data provided on stdin' if STDIN.tty?
contents = STDIN.read
else
fail 'file does not exist' unless File.exists? input
contents = File.read input
end
akerl
reviewed
Sep 1, 2014
| end | ||
| begin | ||
| contents = JSON.parse(contents) |
This comment has been minimized.
This comment has been minimized.
akerl
Sep 1, 2014
Owner
Shadowing variables like this is frowned upon; variables are cheap, I might make them "raw" and "parsed" or similar
akerl
reviewed
Sep 1, 2014
| params = { username: params } unless params.is_a? Hash | ||
| @stats = GithubStats.new(params[:username]) | ||
| params = { user: params } unless params.is_a? Hash | ||
| @stats = params.fetch(:data) { GithubStats.new(params[:user]).data } |
This comment has been minimized.
This comment has been minimized.
akerl
Sep 1, 2014
Owner
I'm not 100% sure if I used attributes of GithubStats in the rest of this lib. If I did, you may need to do some cute magic now that it's a GithubStats::Data object.
This comment has been minimized.
This comment has been minimized.
Stantheman
Sep 1, 2014
Contributor
I've tried to find where it happens, I'll take a second gander. So far in my flexing of it, I haven't seen it
This comment has been minimized.
This comment has been minimized.
Stantheman
added some commits
Sep 1, 2014
This comment has been minimized.
This comment has been minimized.
coveralls
commented
Sep 1, 2014
This comment has been minimized.
This comment has been minimized.
coveralls
commented
Sep 1, 2014
This comment has been minimized.
This comment has been minimized.
coveralls
commented
Sep 1, 2014
Stantheman
added some commits
Sep 1, 2014
This comment has been minimized.
This comment has been minimized.
coveralls
commented
Sep 1, 2014
This comment has been minimized.
This comment has been minimized.
coveralls
commented
Sep 1, 2014
This comment has been minimized.
This comment has been minimized.
|
Welcome to Ruby Club |
added a commit
that referenced
this pull request
Sep 2, 2014
Stantheman commentedSep 1, 2014
This adds a -i/--input flag that takes a file or STDIN. In concert with something like fakehubstats, this would allow githubchart to create graphs of non-public or even non-git related streaks.
This is basically my first foray into Ruby, so I'm looking forward to feedback. I still need to add tests.