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
Improvements and refactoring of help system. #10280
Conversation
[ci skip-rust-tests]
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.
Great!
deprecation_version: Optional[str] | ||
details: Optional[str] | ||
|
||
|
||
# TODO: Get rid of this? The parser should be able to lazily track. | ||
class OptionTracker: |
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.
Is this v1 only?
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.
I've never 100% understood this class, because I thought that the RankedValue
container would be able to record where things had come from. But have never looked deeply.
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.
The RankedValue
is just a single value attached to a Rank, it doesn't know about any other values.
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.
But there's probably some simplification we can do. E.g., the RankedValue can carry the "details" information (e.g., which config file a CONFIG-level value came from)
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.
It is, but it is inside an OptionValueContainer
, which does have all of them, I thought? EDIT: But looking at it now, it's just a dict. Ok.
Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com>
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.
Great cleanup!
deprecation_version: Optional[str] | ||
details: Optional[str] | ||
|
||
|
||
# TODO: Get rid of this? The parser should be able to lazily track. | ||
class OptionTracker: |
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.
I've never 100% understood this class, because I thought that the RankedValue
container would be able to record where things had come from. But have never looked deeply.
Co-authored-by: Eric Arellano <14852634+Eric-Arellano@users.noreply.github.com>
Previously the HelpFormatter, HelpPrinter and HelpInfoExtracter were rather
entangled. This change:
instance of a new AllHelpInfo class, containing all help information,
including scope descriptions. The HelpPrinter/HelpFormatter operate
exclusively on this data. They no longer need access to an Options instance,
let alone a HelpInfoExtractor.
[ci skip-rust-tests]