Provide interface and individual namespaces for brew CLI commands

[dduugg]

• Have you followed the guidelines in our Contributing document?
• Have you checked to ensure there aren't other open Pull Requests for the same change?
• Have you added an explanation of what your changes do and why you'd like us to include them?
• Have you written new tests for your changes? Here's an example.
• Have you successfully run brew style with your changes locally?
• Have you successfully run brew typecheck with your changes locally?
• Have you successfully run brew tests with your changes locally?

---

This is a strawman implementation of #16814. It adds an AbstractCommand, which requires implementing raw_args (usually implemented as {$name}_args, unparsed_args could be a better name) and run (implemented as ${name}). There's also a CommandRegistry to allow lookup by command name (used in brew.rb).

Two commands are ported as examples (one in cmd, one in dev-cmd). They are currently namespaced according to their current paths. (Best viewed with whitespace diffing disabled, due to indentation changes.)

(Tests forthcoming, pending consensus on approach/implementation.)

[MikeMcQuaid]

@dduugg I like the approach, nice work!

[apainintheneck]

I'm not sure this is a great approach.

It seems like we have a base class with a callback to register all classes that inherit from it. Then, we use that registry to find and call the command class that matches the command the user entered. All of that makes sense.

The problem is that we currently don't load most of the commands. In fact, we only load one command per run 99% of the time. That allows us to lazy load a bunch of files based on the command that is getting run. If we have to load all commands into memory before running one of them, we'll be including most of the codebase by default which will likely negatively affect start up time.

I think we should preserve the current lazy loading behavior if possible.

[dduugg]

I think we should preserve the current lazy loading behavior if possible.

There are no changes to the loading behavior, which is still a path search based on the command name. The registry is a convenience for mapping the command name to the class name, once the file has been loaded. I’m open to the critique that it’s over-designed for this use case. (I can take a look at returning the command class more directly as a result of the path resolution search.)

[apainintheneck]

Ah, okay. My bad. I completely misinterpreted what was being proposed here. I'm fine with this approach. It is unfortunate that we'll be up to three different command types but I guess that's the price we pay for backwards compatibility.

[dduugg]

@apainintheneck check out the latest commit. I've done away with the registry in favor of a simple search of AbstactCommand's subclasses (using the new subclasses method introduced in Ruby 3.1). It nicely avoids the rubocop/sorbet issues associated with const_get or ObjectSpace. (It's also O(n), but i don't think that's an issue. 😸)

[apainintheneck]

@apainintheneck check out the latest commit. I've done away with the registry in favor of a simple search of AbstactCommand's subclasses (using the new subclasses method introduced in Ruby 3.1). It nicely avoids the rubocop/sorbet issues associated with const_get or ObjectSpace. (It's also O(n), but i don't think that's an issue. 😸)

This O(n) where n is most likely just 1, right? Or does n refer to all objects. I remember the Rails #descendants method being pretty inefficient because it had to walk the entire ObjectSpace. It's probably fine since we'll only call it once in the lifetime of the program.

I prefer this approach since we don't plan on referring to the registry more than once so we might as well just not build it in the first place. I'm fine with whichever you prefer though.

[dduugg]

This O(n) where n is most likely just 1, right? Or does n refer to all objects. I remember the Rails #descendants method being pretty inefficient because it had to walk the entire ObjectSpace. It's probably fine since we'll only call it once in the lifetime of the program.

I prefer this approach since we don't plan on referring to the registry more than once so we might as well just not build it in the first place. I'm fine with whichever you prefer though.

Sorry for the lack of specificity, I was referring to the command lookup, which loops over subclasses now, instead of using a hash lookup. (I don't know the internals, though I suspect subclasses is much more efficient that what Rails had to work with). I expect them to be indistinguishable in practice.

[dduugg]

I believe initial feedback has been addressed, opening this for general review. (More than happy to port over more commands to demonstrate viability.)

[apainintheneck]

I believe initial feedback has been addressed, opening this for general review. (More than happy to port over more commands to demonstrate viability.)

IMO this is probably enough unless there is a specific command that you think will be especially tricky to port over to the new interface. I think that shouldn't be the case though, right?

[apainintheneck]

This looks great to me.

[MikeMcQuaid]

Looks good to me except for one missing piece with it_behaves_like "parseable arguments".

[dduugg]

Note that once we've ported all the commands, we can/should enable RSpec/DescribeClass (the commands are all currently in the Homebrew namespace, so the current approach makes sense).

[dduugg]

I just realized that we no longer need to thread args through other methods (it's now an ivar, since each command is instance), so I've simplified some method signatures accordingly.

[Bo98]

Feels weaker from a typechecking point of view and feels weird seeing this but still seeing args.formula? and args.verbose?

[apainintheneck]

I'd overlooked this change. Is this needed to make Sorbet happy? If not, the old syntax is preferable.

[dduugg]

Correct. Because CLI::Args subclasses OpenStruct, many of the commands are just hash accessors, and not supported by Sorbet. Thus args, as the result of Parser#parse, has been treated as untyped:

      # @return [Args] The actual return type is `Args`, but since `Args` uses `method_missing` to handle options, the
      #   `sig` annotates this as returning `T.untyped` to avoid spurious type errors.
      sig { params(argv: T::Array[String], ignore_invalid_options: T::Boolean).returns(T.untyped) }
      def parse(argv = ARGV.freeze, ignore_invalid_options: false)

As part of this new implementation, args is correctly typed, but may require changes:

    # @note because `Args` makes use `OpenStruct`, subclasses may need to use a tapioca compiler,
    #   hash accessors, args.rbi, or other means to make this work with legacy commands:
    sig { returns(CLI::Args) }
    attr_reader :args

(Reviewers may have noticed that AbstractCommand has the rare # typed: strong, which is often overkill, but helpful for ancestors or other extensible tooling.)

Some args commands still work, for various reasons. As noted above, args.rbi is one option, but i don't especially love it (and I don't really have the bandwidth to write type signatures as part of the first pass of converting commands). It makes the methods available to all commands, rather than for just the command(s) that define the flag. I'm actively thinking about how to namespace the args as well, as a sort of v2 of what I'm introducing here, but I don't have anything concrete to propose at this time.

[apainintheneck]

The problem here is that while args.pinned? and args[:pinned?] are equivalent the new approach doesn't handle typos well. args.pinnedd? raises an error Error: CLI arg for pinnedd? is not declared for this command while args[:pinnedd?] doesn't fail and just returns nil.

Could we just keep cli args untyped and avoid this change? What additional type safety do we get by typing this attribute?

[apainintheneck]

Also, this is just another example of why a refactor of CLI::Args might be in the cards at some point.

[Bo98]

Sounds like we've moving from something that checked for errors at runtime so something that never checks for errors? I'm not entirely sure what the marginal type improvement here is.

[dduugg]

Sounds like we've moving from something that checked for errors at runtime so something that never checks for errors?

Could you elaborate, possibly with examples? I'm not sure that I understand this claim.

I'm not entirely sure what the marginal type improvement here is.

args was previously untyped by default, but now is typed. This is an issue accessing when OpenStruct hash values via methods.

[Bo98]

diff --git a/Library/Homebrew/cmd/list.rb b/Library/Homebrew/cmd/list.rb
index 238f63f53a..d3f7953824 100644
--- a/Library/Homebrew/cmd/list.rb
+++ b/Library/Homebrew/cmd/list.rb
@@ -96,7 +96,7 @@ module Homebrew
 
           ls_args = []
           ls_args << "-1" if args[:"1?"]
-          ls_args << "-l" if args[:l?]
+          ls_args << "-l" if args.somethinginvalid?
           ls_args << "-r" if args[:r?]
           ls_args << "-t" if args[:t?]

results in:

$ brew list
Error: CLI arg for `somethinginvalid?` is not declared for this command

---

However this:

diff --git a/Library/Homebrew/cmd/list.rb b/Library/Homebrew/cmd/list.rb
index 238f63f53a..f1fee19c14 100644
--- a/Library/Homebrew/cmd/list.rb
+++ b/Library/Homebrew/cmd/list.rb
@@ -96,7 +96,7 @@ module Homebrew
 
           ls_args = []
           ls_args << "-1" if args[:"1?"]
-          ls_args << "-l" if args[:l?]
+          ls_args << "-l" if args[:somethinginvalid?]
           ls_args << "-r" if args[:r?]
           ls_args << "-t" if args[:t?]
 

will result in no error. So we're losing some type checking here (albeit not via Sorbet and more from our special OpenStruct subclass).

To be clear: I do agree with the idea that we should do better namespacing so that we can extend this to work via Sorbet too if possible.

[dduugg]

Perfect answer, thank you for the clear example. I believe the latest commit copies the behavior of method access to hash access, PTAL.

[apainintheneck]

I think that's a fair compromise for now. It'd be nice to make the syntax more consistent at some point but that will likely require a more involved refactor.

[dduugg]

I believe initial feedback has been addressed, opening this for general review. (More than happy to port over more commands to demonstrate viability.)

IMO this is probably enough unless there is a specific command that you think will be especially tricky to port over to the new interface. I think that shouldn't be the case though, right?

I've got a follow-up PR ready with ten commands ported and the only issue has been adding support for CamelCase to kebab-case mapping for multi-word commands.

[dduugg]

Opening this up for re-review, after merging #16880

I've reverted the changes that used hash accessors for accessing args, because a) that style was considered less ergonomic and b) it has a different behavior than method access due to overrides in Args 🙀.

I've also reverted most of the changes to Parser, with the exception of making option_to_name a class method. (This is to avoid generating redundant type signatures for global args for every command).

[dduugg]

Cheating a bit here – command arg methods will be isolated to individual files per command, though (for now) arg methods all live in the same namespace (Homebrew::CLI::Args).

[MikeMcQuaid]

@dduugg Potentially outrageous suggestion that can wait to another PR (including discussion can wait) to not block this one:

What if all the per-command arguments were subclasses of Homebrew::CLI::Args?

[dduugg]

That's the eventual idea, hence the "for now" above. 😉

[MikeMcQuaid]

Looks good! Happy to have you self-merge when you're happy/ready @dduugg!