Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

GPA Improvements: 1.15 to 2.02 #79

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions .codeclimate.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
---
engines:
duplication:
enabled: true
config:
languages:
- ruby

fixme:
enabled: true

rubocop:
enabled: true

ratings:
paths:
- "**.rb"

exclude_paths:
- lib/papertrail/okjson.rb
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good idea

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I usually avoid adding codeclimate.yml (less clutter), but it is needed if we want to exclude the vendored okjson (which pulls the gpa down quite a bit...)

2 changes: 1 addition & 1 deletion Rakefile
Original file line number Diff line number Diff line change
Expand Up @@ -115,7 +115,7 @@ desc "Generate #{gemspec_file}"
task :gemspec => :validate do
# read spec file and split out manifest section
spec = File.read(gemspec_file)
head, manifest, tail = spec.split(" # = MANIFEST =\n")
head, _manifest, tail = spec.split(" # = MANIFEST =\n")

# replace name version and date
replace_header(head, :name)
Expand Down
16 changes: 8 additions & 8 deletions lib/papertrail/cli.rb
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ def initialize
end

def run
if configfile = find_configfile
if configfile
configfile_options = load_configfile(configfile)
options.merge!(configfile_options)
end
Expand All @@ -38,11 +38,11 @@ def run
opts.banner = "papertrail - command-line tail and search for Papertrail log management service"
opts.version = Papertrail::VERSION

opts.on("-h", "--help", "Show usage") do |v|
opts.on("-h", "--help", "Show usage") do
puts opts
exit
end
opts.on("-f", "--follow", "Continue running and printing new events (off)") do |v|
opts.on("-f", "--follow", "Continue running and printing new events (off)") do
options[:follow] = true
end
opts.on("--min-time MIN", "Earliest time to search from") do |v|
Expand All @@ -66,15 +66,15 @@ def run
opts.on("-s", "--system SYSTEM", "System to search") do |v|
options[:system] = v
end
opts.on("-j", "--json", "Output raw JSON data (off)") do |v|
opts.on("-j", "--json", "Output raw JSON data (off)") do
options[:json] = true
end
opts.on("--color [program|system|all|off]",
[:program, :system, :all, :off],
"Attribute(s) to colorize based on (program)") do |v|
opts.on("--color [program|system|all|off]",
[:program, :system, :all, :off], "Attribute(s) to colorize based on (program)") do |v|

options[:color] = v
end
opts.on("--force-color", "Force use of ANSI color characters even on non-tty outputs (off)") do |v|
opts.on("--force-color", "Force use of ANSI color characters even on non-tty outputs (off)") do
options[:force_color] = true
end
opts.on("-V", "--version", "Display the version and exit") do |v|
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's another unused block argument.

Expand Down
4 changes: 2 additions & 2 deletions lib/papertrail/cli_add_group.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,15 +14,15 @@ def run
:token => ENV['PAPERTRAIL_API_TOKEN'],
}

if configfile = find_configfile
if configfile
configfile_options = load_configfile(configfile)
options.merge!(configfile_options)
end

OptionParser.new do |opts|
opts.banner = "papertrail-add-group"

opts.on("-h", "--help", "Show usage") do |v|
opts.on("-h", "--help", "Show usage") do
puts opts
exit
end
Expand Down
4 changes: 2 additions & 2 deletions lib/papertrail/cli_add_system.rb
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ def run
:token => ENV['PAPERTRAIL_API_TOKEN'],
}

if configfile = find_configfile
if configfile
configfile_options = load_configfile(configfile)
options.merge!(configfile_options)
end
Expand Down Expand Up @@ -59,7 +59,7 @@ def run
opts.separator ''
opts.separator "Common options:"

opts.on("-h", "--help", "Show usage") do |v|
opts.on("-h", "--help", "Show usage") do
puts opts
exit
end
Expand Down
8 changes: 6 additions & 2 deletions lib/papertrail/cli_helpers.rb
Original file line number Diff line number Diff line change
@@ -1,10 +1,14 @@
module Papertrail
module CliHelpers
def configfile
@configfile ||= find_configfile
end

def find_configfile
if File.exists?(path = File.expand_path('.papertrail.yml'))
if File.exist?(path = File.expand_path('.papertrail.yml'))
return path
end
if File.exists?(path = File.expand_path('~/.papertrail.yml'))
if File.exist?(path = File.expand_path('~/.papertrail.yml'))
return path
end
rescue ArgumentError
Expand Down
2 changes: 1 addition & 1 deletion lib/papertrail/cli_join_group.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ def run
:token => ENV['PAPERTRAIL_API_TOKEN'],
}

if configfile = find_configfile
if configfile
configfile_options = load_configfile(configfile)
options.merge!(configfile_options)
end
Expand Down
2 changes: 1 addition & 1 deletion lib/papertrail/cli_leave_group.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ def run
:token => ENV['PAPERTRAIL_API_TOKEN'],
}

if configfile = find_configfile
if configfile
configfile_options = load_configfile(configfile)
options.merge!(configfile_options)
end
Expand Down
2 changes: 1 addition & 1 deletion lib/papertrail/cli_remove_system.rb
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ def run
:token => ENV['PAPERTRAIL_API_TOKEN'],
}

if configfile = find_configfile
if configfile
configfile_options = load_configfile(configfile)
options.merge!(configfile_options)
end
Expand Down
45 changes: 20 additions & 25 deletions lib/papertrail/http_client.rb
Original file line number Diff line number Diff line change
Expand Up @@ -42,47 +42,27 @@ def get(path, params = {})
if params.size > 0
path = "#{path}?#{build_nested_query(params)}"
end
attempts = 0
begin

attempt 3, 5.0 do
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it'd be easier to understand if the arguments to attempt were given as a hash (pretty sure we want to continue supporting Ruby 1.9 so it'd actually have to be a Hash and not kwargs 😕).

on_complete(https.get(request_uri(path), @headers))
rescue SystemCallError, Net::HTTPFatalError => e
sleep 5.0
attempts += 1
retry if (attempts < 3)
raise e
end
end

def put(path, params)
attempts = 0
begin
attempt do
on_complete(https.put(request_uri(path), build_nested_query(params), @headers))
rescue SystemCallError, Net::HTTPFatalError => e
attempts += 1
retry if (attempts < 3)
raise e
end
end

def post(path, params)
attempts = 0
begin
attempt do
on_complete(https.post(request_uri(path), build_nested_query(params), @headers))
rescue SystemCallError, Net::HTTPFatalError => e
attempts += 1
retry if (attempts < 3)
raise e
end
end

def delete(path)
attempts = 0
begin
attempt do
on_complete(https.delete(request_uri(path), @headers))
rescue SystemCallError, Net::HTTPFatalError => e
attempts += 1
retry if (attempts < 3)
raise e
end
end

Expand Down Expand Up @@ -156,6 +136,21 @@ def escape(s)
'%' + $&.unpack('H2' * $&.bytesize).join('%').upcase
}.tr(' ', '+')
end

def attempt(tries=3, wait=nil)
return unless block_given?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is an internal method to this class, I think it's fine to let this method blow up when calling yield rather than returning if there's no block. If someone calls this method without a block, it's definitely an error. I could also see raising ArgumentError. I don't really have a preference.


attempts = 0
begin
yield
rescue => e
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We'll want to continue rescuing SystemCallError, Net::HTTPFatalError. I think it makes sense to encode that here in this method and not supplied as arguments. Maybe that means this method is really #attempt_http or the like.

sleep wait if wait
attempts += 1
retry if (attempts < tries)
raise e
end
end

end

end