-
-
Notifications
You must be signed in to change notification settings - Fork 9.8k
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
Remove OpenStruct from CLI::Args #18847
Conversation
Promising start, passes tests locally, but has an issue where methods are being defined twice – once when the cmd_args are read, and once when the args are set. I should be able to refine this approach by defining and setting attributes instead, then we'll see what the next blocker is. |
203bd8a
to
a738f8d
Compare
Marking as ready for review. Hopefully this is a reasonable, incremental improvement over the previous code. |
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.
Thanks @dduugg! Looks good to me to merge when you're around for a few hours to deal with any post-merge 💥
brew style
with your changes locally?brew typecheck
with your changes locally?brew tests
with your changes locally?Resolves #18777
@table
attr that OpenStruct used, but it is now exclusively for args (not metadata likenamed
andremaining
). I'm open to renaming the attr.missing_method
dispatch is removed, only defined methods are allowedset_arg
method is provided, to allow setting args from the parser. There is no corresponding getter, because I want to encourage explicit invocations. The usual ruby escape hatches (e.g.instance_variable_get
) are available if needed 😞.@table
lookups, because the methods aren't necessarily defined. It's a bit of an eyesore, but IMO preferable to defining them unnecessarily, or usingrespond_to?
guards.