🌈 Allow options to be set back to defaults#713
Closed
timothysmith0609 wants to merge 3 commits into
Closed
Conversation
ea4e08e to
d8bf01d
Compare
1 task
This was referenced Feb 11, 2020
Contributor
Author
|
Closing as the actual error was in our application (we were calling CLI methods at the class, not object, level) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
repeatableis 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
parseis 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.