Skip to content

🌈 Allow options to be set back to defaults#713

Closed
timothysmith0609 wants to merge 3 commits into
rails:masterfrom
timothysmith0609:fix_repeatable_persistence_bug
Closed

🌈 Allow options to be set back to defaults#713
timothysmith0609 wants to merge 3 commits into
rails:masterfrom
timothysmith0609:fix_repeatable_persistence_bug

Conversation

@timothysmith0609

@timothysmith0609 timothysmith0609 commented Feb 7, 2020

Copy link
Copy Markdown
Contributor

Why do we need to be able to reset options?

#674 introduced a subtle bug that occurs if a Thor command is invoked multiple times during its lifetime (this will most likely only occur when running test suites). The failure mode in this case is that the previously parsed options are not cleared, and instead persisted to the next invocation of a Thor command.

For a concrete example: the krane tests were failing due to filename args being persisted and accumulated across tests.

The offending lines are options.rb#L136-L140, where we merge, instead of overwrite when repeatable is true.

The most straightforward solution seems to be adding this escape hatch that will reset options back to defaults and allow for reparsing from scratch. Let me know if that's a satisfactory approach.

An alternative solution could be to always reset options back to defaults every time parse is called. I chose not to do this since the existing test indicates that was not desired behaviour. This approach has the benefit of taking the onus off the caller to be mindful of clearing options.

  • I have added tests for this

@timothysmith0609 timothysmith0609 force-pushed the fix_repeatable_persistence_bug branch from ea4e08e to d8bf01d Compare February 7, 2020 20:17
@timothysmith0609 timothysmith0609 mentioned this pull request Feb 7, 2020
1 task
@timothysmith0609

Copy link
Copy Markdown
Contributor Author

Closing as the actual error was in our application (we were calling CLI methods at the class, not object, level)

@timothysmith0609 timothysmith0609 deleted the fix_repeatable_persistence_bug branch February 14, 2020 19:52
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.

1 participant