Add tapioca compiler for Homebrew::CLI::Args #16880
Conversation
|
This gives me inspiration and avenues to debug my current non-working branch for a Tapioca compiler for Tty, to replace the generator script, so thank you! |
| :formulae_all_installs_from_args, | ||
| :reproducible_gnutar_args, | ||
| :tar_args, |
There was a problem hiding this comment.
I would support renaming these if it made life easier, FYI.
There was a problem hiding this comment.
It should be nearly as fast to just port the impacted methods to use AbstractCommand (🤞)
\o/ (feel free to share your branch if you want to collaborate) |
|
Ok, I've updated #16815 to validate that this approach removes the need to make changes to either Library/Homebrew/cli/args.rb or how |
| :formulae_all_installs_from_args, | ||
| :reproducible_gnutar_args, | ||
| :tar_args, |
There was a problem hiding this comment.
It should be nearly as fast to just port the impacted methods to use AbstractCommand (🤞)
| comma_array_methods = comma_arrays(parser) | ||
| args = parser.instance_variable_get(:@args) | ||
| args.instance_variable_get(:@table).each do |method_name, value| | ||
| # some args are used in multiple commands (this is ok as long as they have the same type) |
There was a problem hiding this comment.
We could add some validation that method signatures are unchanged, but this won't be a problem once we have namespaced args
| parser = Homebrew.method(args_method_name).call | ||
| comma_array_methods = comma_arrays(parser) | ||
| args = parser.instance_variable_get(:@args) | ||
| args.instance_variable_get(:@table).each do |method_name, value| |
There was a problem hiding this comment.
We're pretty deep in the internals of the parser at this point (accessing an ivar of an ivar), it might be a good idea to write tests and/or update CLI::Parser to more directly expose its method table.
There was a problem hiding this comment.
Those sound like good ideas. If we expose :@table, we should make sure that it's read only.
There was a problem hiding this comment.
Ok, I broke up the logic in order to test various aspects of this compiler. (I decided not make any changes to CLI::Parser). PTAL.
| @@ -2,21 +2,3 @@ | |||
|
|
|||
| # This file contains temporary definitions for fixes that have | |||
| # been submitted upstream to https://github.com/sorbet/sorbet. | |||
|
|
|||
| # https://github.com/sorbet/sorbet/pull/7650 | |||
There was a problem hiding this comment.
unrelated cleanup, while I'm in the sorbet dir (the linked upstream PRs have merged)
| parser = Homebrew.method(args_method_name).call | ||
| comma_array_methods = comma_arrays(parser) | ||
| args = parser.instance_variable_get(:@args) | ||
| args.instance_variable_get(:@table).each do |method_name, value| |
There was a problem hiding this comment.
Those sound like good ideas. If we expose :@table, we should make sure that it's read only.
| # FIXME: Enable cop again when https://github.com/sorbet/sorbet/issues/3532 is fixed. | ||
| # rubocop:disable Style/MutableConstant | ||
| ConstantType = type_member { { fixed: T.class_of(Homebrew::CLI::Args) } } | ||
| # rubocop:enable Style/MutableConstant |
There was a problem hiding this comment.
I don't follow where you use this -- care to elaborate?
There was a problem hiding this comment.
Per Tapioca docs:
Every compiler must declare the type member ConstantType in order for Sorbet to understand what the return type of the constant attribute reader is. It needs to be assigned the correct type variable matching the type of constants that gather_constants returns. This generic variable allows Sorbet to type-check method calls on the constant reader in your decorate method. See the Sorbet documentation on generics for more information.
|
I believe feedback has been addressed, I'm going to smash that green button and resume work on the related PR. Feel free to comment on that one if I've missed anything here, and I'll address over there. 🙇♂️ |
brew stylewith your changes locally?brew typecheckwith your changes locally?brew testswith your changes locally?See context at #16815 (comment)
I wanted to put this off a bit, since currently all command args live in the same namespace. That means that any arg accepted by any command will typecheck when used in any other command. Thus, this PR adds an RBI that allows 297 methods on
Homebrew::CLI::Args.However, this should allow us to roll out the
AbstractCommandinterface without resorting to hash accessors or weakening the contract that the interface provides.I'm opening this up as a draft for initial feedback. I will see how it integrates with the PR linked above as time allows in the coming days.