-
Notifications
You must be signed in to change notification settings - Fork 43
Add #pretty_print which prints a nice table of data #37
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
Conversation
@charliesome: I discovered a bug in
That method should return |
@parkr Yup, you're right. Looks like
|
That doesn't look right. Shouldn't |
Ha. It's a long story. I'm an edge case. But yes, |
@benbalter Well, ok. I think it'll work in the vast majority of cases then. How do you like my new table? I think it reads more easily than the |
ip_problems << "not apex domain" if not values[:apex_domain?] | ||
ip_problems << "invalid domain" if not values[:valid_domain?] | ||
ip_problems << "old ip address used" if values[:old_ip_address?] | ||
output.puts new_line "IP Problems", "#{ip_problems.size > 0 ? ip_problems.join(", ") : "none"} " |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this only be printed if :should_be_a_record?
is true? Or always print it?
@parkr Nice work. A few thoughts:
|
The purpose of this PR is to improve upon the readability of
Yeah, but I didn't want to add another dependency. If you think it's ok, then I can use that instead.
Yes, this function encapsulates the various nuances of that map, e.g. when Y influences the map value of X. It's not straight 1-1 mapping so I didn't go that route. I could create a TablePrinter or use Terminal Table to simplify the formatting logic if you'd like. |
Merged in master, which contains #39. Easy peasy. |
@@ -56,6 +56,7 @@ raises GitHubPages::HealthCheck::Errors::InvalidCNAMEError | |||
:pages_domain?=>false, | |||
:valid?=>true | |||
} | |||
> require 'json' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@benbalter is this needed still?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As currently coded, yes, JSON isn't required by default, but a method calls to_json
, which would result in an error if not included. My thinking was, if you require json
, to_json
would work (just like for native objects), and if you don't you'll get an undefined method to_json
error. Does that make sense? (use case being a simple API, that could theoretically just call to_hash.to_json
itself).
@@ -9,8 +9,6 @@ | |||
require "resolv" | |||
require "timeout" | |||
require "octokit" | |||
require "json" | |||
require "yaml" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If using the YAML approach for to_s
, you'll want to re-require yaml
(or maybe lazy require it in the method?)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added in e36ec9b
@benbalter Good to merge? |
|
Add #pretty_print which prints a nice table of data
This PR introduces a
Printer
which can print either the old way or the new way. The new way is a nice table:What do you think?
/cc @github/pages