Add named_args DSL for commands

[Rylan12]

• 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?
• Have you successfully run brew man locally and committed any changes?

---

See #10229 and #10229 for some more context/discussion.

This PR refactors the NamedArgs class and replaces the named, min_named, and max_named methods with a single named_args method.

The named_args method takes a symbol describing the type of named arguments that should be accepted. These can be anything. The method can also define the number of named arguments allowed using the named, min, and max parameters. The main benefit of this new method is that it will allow shell completions to be generated automatically for all Homebrew commands.

However, another benefit is that converting named arguments to the desired type is easier. I've replaced the args.named.to_* methods with args.named.convert and args.named.convert_to_paths.

For example, calling named_args :formula and then args.named.convert will convert all named arguments to formula objects.

---

This PR is still a work in progress. I've yet to implement a few features of the named_args command and there are a few more internal commands that will need to be updated. I'm keeping track of which commands I've updated and what each of their constraints are in this spreadsheet.

Additionally, brew typecheck is failing and I could use some help resolving those issues.

Lastly, I know that brew tests fails at the moment. They're mostly timeout exceptions which makes me think that something isn't working right with the new methods. Regardless, I just wanted to get this up for initial feedback first.

[BrewTestBot]

Review period will end on 2021-01-12 at 06:28:12 UTC.

[MikeMcQuaid]

This PR refactors the NamedArgs class and replaces the named, min_named, and max_named methods with a single named_args method.

The existing methods are still used in (our) taps and therefore probably other taps too so need to be kept around in some form for compatibility reasons.

Otherwise: looking great so far, thanks for this!

[MikeMcQuaid]

Should this raise if this value is invalid?

[Rylan12]

Under my current thinking: no.

We do want to allow named arguments that don't fall into any of the categories. I also current used named_args :command and named_args :diagnostic_check for some commands. These symbols will cause the args to be treated simply as text, but will (in the future) allow completions to autofill commands or diagnostic checks.

[Rylan12]

Although, on second thought, args.named should be sufficient in these cases and we probably shouldn't be calling args.named.convert, so maybe that's a good idea.

[MikeMcQuaid]

These symbols will cause the args to be treated simply as text, but will (in the future) allow completions to autofill commands or diagnostic checks.

I feel like we can add this in future when we want but "treating symbols as text" seems overly permissive for now.

[MikeMcQuaid]

to_objects or something feels like a nicer method name. I wonder if to_formulae_and_casks or something could be used and just change the API?

Also, this method feels really big; maybe it's worth breaking down a bit?

[Rylan12]

The reason I wanted to move away from to_* was that I wanted this method to be a catch-all. One could always call args.named.convert and get the converted objects. Maybe that's not the way to go about this.

I'd be in favor of breaking it down a little, but I think the main reason it's so long is it has a huge case statement. It already calls out to helper methods for the actual arg conversion.

[reitermarkus]

I feel like having the return type of this depend on what is passed to CLI::Parser is too implicit (also no way to have a meaningful type signature).

Maybe we could store arguments as Arg objects which have e.g. a to_cask method, etc., so the API would look like:

args.named.map(&:to_cask)

[MikeMcQuaid]

I feel like having the return type of this depend on what is passed to CLI::Parser is too implicit (also no way to have a meaningful type signature).

Agreed 👍🏻

Maybe we could store arguments as Arg objects which have e.g. a to_cask method, etc., so the API would look like:

args.named.map(&:to_cask)

Yeh, something like this looks nice. I'm basically wanting to have "if I want to get casks the call method/parameters contain the word cask"

[Rylan12]

Agreed, something like this is a better solution. Let me give some more thought to this and whether this is actually any better than the existing args.named.to_* methods. We were using those successfully before so I don't think we need to reinvent them just to allow this named_args method.

That's why this is a draft PR. I'm still working through everything and figuring out what the best way to do it is

[MikeMcQuaid]

That's why this is a draft PR. I'm still working through everything and figuring out what the best way to do it is

Yup, all good ❤️

[MikeMcQuaid]

We were using those successfully before so I don't think we need to reinvent them just to allow this named_args method.

Potentially they could be extended to raise if called in the wrong way (i.e. if named_args :tap and you request named.to_formulae or something?).

[Rylan12]

This PR refactors the NamedArgs class and replaces the named, min_named, and max_named methods with a single named_args method.

The existing methods are still used in (our) taps and therefore probably other taps too so need to be kept around in some form for compatibility reasons.

Yes, that's why I added the commented-out deprecation lines on them. They haven't actually been removed yet (and will still work fine). I just layed the ground work for removing them.

[Rylan12]

Thanks for the feedback. More generally, is this the right way to proceed?

I was picturing args.named.convert would be a catch-all conversion method that would replace the args.named.to_* methods. My reasoning was that with the current system, each time we needed a new combination we would need to methods and would end up with a huge number of one-use methods like them. Using convert doesn't restrict anyone from using only the methods we've already defined. However, it also seems that we have covered the bases pretty well at least for internal use, so maybe that's just overkill.

[BrewTestBot]

Review period ended.

[Rylan12]

I decided that what I really want from this PR is the named_args method. Adding that sets us up well for the completions.

I reverted the args.named.convert change for now as I feel like we don't need to modify working functionality. Don't worry, though, I have a backup in a branch in my fork.

---

A few questions: should there be a difference between :installed_formula and :keg? Right now I opted for :keg in all places, but I think :installed_formula is clearer in some cases (like uninstall). Especially if these are going to dictate the error messages as I've implemented. In terms of completions, they can be treated the same.

I've been operating under the assumption that all kegs are installed formulae. Is that fair or are there instances where kegs aren't formulae?

Separately, what's the difference between args.named.to_formulae and args.named.to_resolved_formulae?

[MikeMcQuaid]

I decided that what I really want from this PR is the named_args method. Adding that sets us up well for the completions.

👍🏻

A few questions: should there be a difference between :installed_formula and :keg? Right now I opted for :keg in all places, but I think :installed_formula is clearer in some cases (like uninstall). Especially if these are going to dictate the error messages as I've implemented. In terms of completions, they can be treated the same.

Yes, I think now that we've removed diy we can remove the references to kegs when not necessary (as long as the underlying code can handle e.g. uninstalling kegs with no remaining formulae).

I've been operating under the assumption that all kegs are installed formulae. Is that fair or are there instances where kegs aren't formulae?

brew diy was one break for that assumption but the main one is uninstalling formulae that have been deleted/untapped.

Separately, what's the difference between args.named.to_formulae and args.named.to_resolved_formulae?

The latter uses Formulary.resolve which sets the active_spec and build based on the tab. In short, this handles installed HEAD formulae properly.

[Rylan12]

Yes, I think now that we've removed diy we can remove the references to kegs when not necessary (as long as the underlying code can handle e.g. uninstalling kegs with no remaining formulae).

Okay. For this PR, I think I'd rather continue to use .to_kegs and those methods. In the future, though, they should probably be replaced with .to_installed_formulae or something like that. I'll change all the named_args that reference :keg to :installed_formula. I think it makes more sense in almost all cases. It doesn't have any functional change to how arguments are parsed/converted. The only difference is in the error message that's shown if no named arguments are passed when required. Eventually, it will affect completions, too, but not at the momen.

brew diy was one break for that assumption but the main one is uninstalling formulae that have been deleted/untapped.

Yeah, I kind of figured brew diy was an exception but I never explored it when it was un-disabled.

The latter uses Formulary.resolve which sets the active_spec and build based on the tab. In short, this handles installed HEAD formulae properly.

When would you want to use Formulary.resolve over Formulary.factory?

[Rylan12]

Actually, there is one small difference. Currently, we calculate installed formulae differently in completions. In bash we use ls "$(brew --cellar)". In fish and zsh, we use brew list --formula.

I believe what you're saying is that brew list will not show kegs of formulae that have been removed while ls "$(brew --cellar)" will. I guess the question becomes:

Should completions complete installed formulae from brew list --formula, ls "$(brew --cellar)", or both depending on context? If it's not the latter, we probably don't need to use named_args :installed_formula and named_args :keg. We can silently include or ignore non-formulae kegs in the completions. If they should both be used, we can use :keg to indicate ls "$(brew --cellar)" and :installed_formula to indicate brew list.

ls "$(brew --cellar)" is much faster so I figure that is the best way to do it in completions. It has the added benefit of completing non-formula kegs (which it sounds like are very rare) for things like uninstall.

[MikeMcQuaid]

Okay. For this PR, I think I'd rather continue to use .to_kegs and those methods. In the future, though, they should probably be replaced with .to_installed_formulae or something like that. I'll change all the named_args that reference :keg to :installed_formula. I think it makes more sense in almost all cases. It doesn't have any functional change to how arguments are parsed/converted. The only difference is in the error message that's shown if no named arguments are passed when required. Eventually, it will affect completions, too, but not at the momen.

Fine with me 👍🏻

When would you want to use Formulary.resolve over Formulary.factory?

grepping for usage may help but I'd say: when you care about whether the installed formula was --HEAD or not e.g. on upgrades.

I believe what you're saying is that brew list will not show kegs of formulae that have been removed while ls "$(brew --cellar)" will.

I'm not sure, depends on the implementation (and perhaps even the other flags passed 😭).

We can silently include or ignore non-formulae kegs in the completions. If they should both be used, we can use :keg to indicate ls "$(brew --cellar)" and :installed_formula to indicate brew list.
ls "$(brew --cellar)" is much faster so I figure that is the best way to do it in completions. It has the added benefit of completing non-formula kegs (which it sounds like are very rare) for things like uninstall.

Sounds good 👍🏻

[Rylan12]

When would you want to use Formulary.resolve over Formulary.factory?

grepping for usage may help but I'd say: when you care about whether the installed formula was --HEAD or not e.g. on upgrades.

Maybe what I meant to ask is: "is there ever a situation where you would need to use Formulary.factory and Formulary.resolve will not work as expected?

I just quickly grepped and it seems the only two uses of Formulary.resolve are in named_args and Cleanup.

I'm not worrying too much about it for now, but I wonder if Formulary.resolve can be used in all cases which would mean we don't need to_formulae and to_resolved_formulae. My guess, though, is that we do need them both because they've both been created... That's probably a discussion for the future.

---

Okay, it sounds like we're on the same page in terms of the path of this PR. I think it's ready for now. One thing I definitely want to wait for before merging is the brew completions command to be added so I can add it here.

Edit: oh, and CI is failing. I'll resolve that too 😅

Edit 2: done!

[MikeMcQuaid]

I'm not worrying too much about it for now, but I wonder if Formulary.resolve can be used in all cases which would mean we don't need to_formulae and to_resolved_formulae. My guess, though, is that we do need them both because they've both been created... That's probably a discussion for the future.

Yeh, it may be it can be used globally. Certainly the naming is not exactly transparent as to what it's doing. Would be a good thing to test (maybe with an environment variable to feature flag it?).

[MikeMcQuaid]

Fantastic work as usual @Rylan12. A few more thoughts/comments but nothing blocking here; merge when you're happy with it.

[Rylan12]

Thinking ahead a little bit: the named, min_named, and max_named methods will be deprecated in the next major/minor release, and this is likely to affect third-party taps with external commands. Additionally, I expect that soon we will be able to generate completions automatically (that's part of the purpose of this PR). Part of that will probably be to add a developer command to generate these completions for a tap's external command.

I'm thinking about writing a blog post when all of that is completed with instructions explaining both how to migrate to using named_args and generating those completions. Does that seem like it would be helpful? If there are very few taps with external commands that use the old named methods, maybe it's not worth it.

[MikeMcQuaid]

Thinking ahead a little bit: the named, min_named, and max_named methods will be deprecated in the next major/minor release, and this is likely to affect third-party taps with external commands. Additionally, I expect that soon we will be able to generate completions automatically (that's part of the purpose of this PR). Part of that will probably be to add a developer command to generate these completions for a tap's external command.

👍🏻 sounds good.

I'm thinking about writing a blog post when all of that is completed with instructions explaining both how to migrate to using named_args and generating those completions. Does that seem like it would be helpful? If there are very few taps with external commands that use the old named methods, maybe it's not worth it.

Sounds helpful but I'm not sure there's many external taps that'd use this, indeed. Could just be something on docs.brew.sh perhaps?

[Rylan12]

Makes sense. I noticed that in the existing external command docs, the recommendation is to use #: comments. Should that be changed to instead use Homebrew::CLI::Parser?

changes addressed

[Rylan12]

I'm going to go ahead and merge this for now. I've got some time tonight and want to work on the completions which requires this PR.

I've made some documentation changes locally and will open a separate PR for those so this isn't held up.