Skip to content
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

🌈 Need to dup default or else we have a troublesome alias #715

Merged
merged 2 commits into from
Feb 28, 2020

Conversation

tsontario
Copy link
Contributor

@tsontario tsontario commented Feb 27, 2020

Following work on Shopify/krane#697, I discovered a subtle bug that is now causing issues with the repeatable flag feature.

If a default argument for a Thor::Option | Thor::Argument is provided, the option/argument constructor will call:

@assigns[argument.human_name] = argument.default

This aliases, and does not create a copy of, argument.default to @assigns[argument.human_name]. So now every time we update @assigns, we are also mutating argument.default. This was never a problem before because, without a repeatable flag, we'd never encounter a failure mode.

TODO

  • unit test

@tsontario tsontario changed the title Need to dup default or else we have a troublesome alias 🌈 Need to dup default or else we have a troublesome alias Feb 27, 2020
Comment on lines 35 to 37
rescue TypeError # Compatibility shim for un-dup-able Fixnum in Ruby < 2.4
@assigns[argument.human_name] = argument.default
next
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before Ruby 2.4, attempting to dup a Fixnum results in a TypeError. From what I can see, that's the only accepted type for an option that could possibly throw, so for the sake of backward's compatibility I've had to add this in

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we need to call next?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, you're right we don't. I'll remove

Add test

add handler

continue loop after rescue

remove byebug history

spelling
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants