Add output formatters #48
Conversation
lib/seafoam/commands.rb
Outdated
|
|
||
| if first == '--json' | ||
| formatter_module = Seafoam::Formatters::Json | ||
| args.drop(1) |
There was a problem hiding this comment.
I don’t understand why this line is here. Is it left behind from an earlier attempt to strip the '--json'?
There was a problem hiding this comment.
I can revisit all of this. But, the context is that Seafoam doesn't use OptionParser to parse its arguments. I did it this way just to get the flag without impacting the rest of the argument processing. I think longer term, adopting OptionParser or maybe even something like Thor would be better.
There was a problem hiding this comment.
But args.drop(1) doesn’t do anything.
There was a problem hiding this comment.
Ahh, I see. Sorry for missing that. The next line reassigns the values and that's all that's needed. It looks like this is just leftover garbage from when I was going back and forth between options. I'll clean that up.
| file, *rest = parse_name(name) | ||
| raise ArgumentError, 'info only works with a file' unless rest == [nil, nil, nil] | ||
|
|
||
| raise ArgumentError, 'info does not take arguments' unless args.empty? |
There was a problem hiding this comment.
Why don’t we want to raise this error any more?
There was a problem hiding this comment.
I don't really recall. I think I changed it because the command processor never passes arguments down. But, I'll switch it back and potentially revisit when reworking argument processing.
lib/seafoam/formatters/base.rb
Outdated
| module Formatter | ||
| def format | ||
| raise NotImplementedError | ||
| end | ||
| end |
There was a problem hiding this comment.
It's my attempt at documenting an interface for what a formatter is. I find seeing a NotImplementedError more useful than NoMethodError. But, if it's too "out there", I can drop it.
There was a problem hiding this comment.
It’s not particularly “out there” but it’s also not doing anything for us yet. No big deal, either seems fine to me. (I know that some people feel more strongly about reserving NotImplementedError for specifically “when a feature is not implemented on the current platform” rather than missing methods in general, but I’m pretty sure MRI doesn’t care.)
There was a problem hiding this comment.
Interesting. I've never seen that distinction before. It's probably selection bias, but I've mostly seen it to mean "yes, this method does exist but you're either calling it on the wrong object or it's not implemented for this object" to avoid any confusion around NoMethodError not referring a typo.
I'll circle back to this afterwards. For now, I'm finding it helpful to distinguish between commands that haven't been updated for the formatter system at all and those that have, but haven't been fully implemented yet.
There was a problem hiding this comment.
I've removed its usage in 831f13d. I'll have to consider usages of NotImplementedError in the future. Thanks for alerting me to that.
| parser = BGV::BGVParser.new(file) | ||
| major, minor = parser.read_file_header(version_check: false) | ||
| @out.puts "BGV #{major}.#{minor}" | ||
| formatter = formatter_module::InfoFormatter.new(major, minor) |
There was a problem hiding this comment.
I like this design — I think it makes sense to pick a formatter namespace and then resolve the relevant constants within it. It’s a little unconventional (vs sending messages to an object) but it’s low-ceremony and it interacts nicely with autoloading.
| module Seafoam | ||
| # Formatters are the mechanism by which `seafoam` command output is presented to the user. | ||
| module Formatters | ||
| autoload :Base, 'seafoam/formatters/base' |
There was a problem hiding this comment.
This is always needed isn't it?
There was a problem hiding this comment.
It's not needed for --help or --version, but those are limited cases. I can unconditionally require if you'd prefer.
|
Idea looks good. Just needs the comments going through. |
nirvdrum
force-pushed
the
json-output
branch
4 times, most recently
from
831f13d
to
66b6f8b
Compare
The primary goal of this PR is to output Seafoam's results in a machine readable format for automation use cases, while not compromising the ease-of-use afforded by the current plain text output. In order to achieve that, I'm proposing a new formatting system with plain-text and JSON formatters to start. It may very well be the case that we never need another format, but if we do, it should slot in easily enough.