-
Notifications
You must be signed in to change notification settings - Fork 5
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
Fi 2962 execute outputters #541
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #541 +/- ##
=======================================
Coverage 84.06% 84.06%
=======================================
Files 262 262
Lines 11410 11410
Branches 1254 1254
=======================================
Hits 9592 9592
Misses 1808 1808
Partials 10 10
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
Gemfile
Outdated
end | ||
gem "tty-spinner", "~> 0.9.3" |
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.
This would have to go in the gemspec.
lib/inferno/apps/cli/execute.rb
Outdated
inflections.acronym 'JSON' | ||
end | ||
|
||
OUTPUTTER_WHITELIST = %w[console plain json quiet].freeze |
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.
This is all fine, but a Hash mapping the option to the outputter class would be a lot simpler. I don't think being dynamic buys us much here, since we would need to alter code here, as well as documentation when adding new outputters anyway.
|
||
module Inferno | ||
module CLI | ||
class Execute | ||
# @private | ||
class ConsoleOutputter | ||
COLOR = Pastel.new | ||
class ConsoleOutputter < JSONOutputter |
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.
Unless I'm missing something, I don't think it makes sense for this to inherit from JSONOutputter.
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.
It inherits serialize
|
||
def print_error(_options, exception) | ||
puts "Error: #{exception.full_message(highlight: false)}" | ||
puts exception.backtrace&.join('\n') |
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.
Why is this different from the console outputter version? Also, doesn't full_message
already include the stack trace?
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.
Plain outputter needs highlight: false
or it outputs ANSI code \e[1
(to make it bold).
But the backtrace part was redundant so I'm removing that.
class Execute | ||
# @private | ||
class QuietOutputter | ||
# rubocop:disable Lint/UnusedMethodArgument |
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.
Don't disable this rule. Prefix the arguments with underscore if they're not used.
Summary
To inferno execute add:
--outputter
option that defaults toconsole
JSON Output format (for dev_validator suite):
Testing Guidance
bundle install
bundle exec inferno services start