Convert some dev commands to use AbstractCommand #16921
Conversation
| @command_name = cmd.command_name | ||
| @is_dev_cmd = cmd.name&.start_with?("Homebrew::DevCmd") | ||
| else | ||
| # FIXME: remove once commands are all subclasses of `AbstractCommand`: |
There was a problem hiding this comment.
This is an especially nice change, IMO. No more inspecting the call stack to figure out the command!
|
|
||
| begin | ||
| Homebrew.send(cmd_args_method_name) if require?(cmd_path) | ||
| if require?(cmd_path) | ||
| cmd = Homebrew::AbstractCommand.command(cmd_name) |
There was a problem hiding this comment.
Can be done in another PR: I think it would make sense to move AbstractCommand to Homebrew::CLI::AbstractCommand.
There was a problem hiding this comment.
I have no strong feelings on their either way.
| module DevCmd | ||
| class BumpRevision < AbstractCommand |
There was a problem hiding this comment.
Similar to above: I think it would make sense to move this either to Homebrew::CLI::BumpRevisionCommand (possibly with self.class.dev_command?), or to Homebrew::CLI::DevCmd::BumpRevision.
There was a problem hiding this comment.
Possibly, feel free to put something together. Currently, Parser and AbstractCommand are tightly coupled in a way that should be avoided, Args still needs to be individually namespaced, commands need to be ported, etc., and I would instead encourage prioritizing that work instead.
I thought about a dev_cmd DSL. I went with the namespace approach rather than introducing more code, but the latter is probably preferred from a classic CS perspective…
There was a problem hiding this comment.
Homebrew::CLI::BumpRevisionCommand(possibly withself.class.dev_command?), or toHomebrew::CLI::DevCmd::BumpRevision.
👎🏻 from me, I prefer the DevCmd namespacing as it:
- better mirrors what's on disk
- has fewer nested modules than the second option
There was a problem hiding this comment.
better mirrors what's on disk
Renaming the files to follow the module naming would naturally be the consequence.
There was a problem hiding this comment.
I would rather keep the files in the same place they have been for a long time and make the modules mirror that rather than move stuff around unnecessarily.
brew stylewith your changes locally?brew typecheckwith your changes locally?brew testswith your changes locally?This PR ports the first ten dev commands (alphabetically) to using AbstractCommand. (There are some minor changes to the Parser and AbstractCommand to better facilitate this.) I did some misc cleanup along the way (moving constants from top-level to inside the command class, using the args ivar instead of passing through to helper methods, making helper methods private, etc.)
I recommend reviewing with whitespace diffs disabled.