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

Conversation

DannyBen
Copy link
Contributor

@DannyBen DannyBen commented Aug 9, 2016

An assortment of small to medium changes to improve code standards:

  1. Exclude okjson from Code Climate
  2. Remove / rename unused variables, mostly in the OptionsParser blocks
  3. Add configfile method to CliHelpers to avoid repeating the confusing condition+assignment if configfile = find_configfile
  4. Reduce code duplication in http_client by adding an attempt times, wait, &block method to handle HTTP retries in a single place.

CC: #78

@lmarburger lmarburger self-assigned this Aug 9, 2016
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.

@lmarburger
Copy link
Contributor

Looks great!

gj1

@DannyBen
Copy link
Contributor Author

Agree with everything, had the same thoughts on all subjects. The global rescue in the attempt method is a mistake on my part. Will update later today.

- "**.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...)

@lmarburger
Copy link
Contributor

I'll give this a run through next week before merging and cutting a release.

@lmarburger lmarburger removed their assignment Dec 19, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants