New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
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! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good so far to me, thanks @dduugg!
:formulae_all_installs_from_args, | ||
:reproducible_gnutar_args, | ||
:tar_args, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would support renaming these if it made life easier, FYI.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't follow where you use this -- care to elaborate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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. 🙇♂️ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, thanks @dduugg!
brew style
with your changes locally?brew typecheck
with your changes locally?brew tests
with 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
AbstractCommand
interface 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.