Streamline option flag management#3641
Open
kdeldycke wants to merge 1 commit into
Open
Conversation
9ca44ec to
3a9316a
Compare
3a9316a to
3b743b8
Compare
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.
This refactor is an attempt in streamlining the code around flag management in
Option.I hope this is not too big to digest. I did not went too far and just focused on
Optioninternals and flag-management. My goal is to colocate decision code by domain. Like having all prompt-related code in the same block. Or all type-resolution code together. Sometimes in the same code block, sometimes under a private utility function. My heuristic is mostly driven by the number of lines and the content of the docstring.Another idiom I introduced was using
@propertyforis_bool_flag. I didn't see that much propoerties used in Click, but these are easier to reason about for me.I also used the oportunity to introduce a type inference point in
_pick_typewhich is a small step echoing what @sirosen was proposing last year about having a separation between "data which is stored (where you might have UNSET) from the data which is viewed". That also resurface the infamousUNSETa bit, but hidden into a newflag_activation_valueproperty. And_hide_unsetis a way to delimit the internals state from the presetation state.This also contains some small syntax cleanups that were flagged by my linters.
Proof that this is the right time to introduce this refactor: after accumulating a lot of unitetsts over the past few years, I did not break any. And I did not discovered any new blind spot in the test suite.
I hope this will make code easier to read and maintain. But I need other eyes to check that.
My initial goal was way too ambitious as I want to create a
FlagOptionsubclass ofOption. But this is way too big and not sure I can pull it yet. So this is like a first step in that general direction.