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

Feat #89 isolation and extensibility design #92

Conversation

BrutalSimplicity
Copy link
Contributor

This is a large PR, and for that I apologize. I'm happy to make changes and provide additional explanation based on feedback.

The main focus of this PR is to add support for isolating Rich Click's formatting and help configuration. The design supports isolation of these components by:

  1. Extending Click's HelpFormatter class
  2. Creating a HelpConfiguration class that doubles the current module-level settings
  3. Adding a decorator that allows the HelpConfiguration to be passed into Click via the supported context_settings argument provided by the Command and Group classes.

Since this package does have an established user base I attempted to maintain the module-level settings behavior to prevent any breaking changes. I also added some tests to help verify the test suite using pytest and pytest-cov for test coverage. The tests are the bulk of this change as I used a snapshot-based approach to capture the input and output of the examples, and then validate their compatibility between module-level and context-level configuration.

I got it a bit carried away and added a few other nuggets since they seemed closely related.

  1. The Rich Console object can also be configured per command and is distinct from the Console instance used internally by the formatter. The RichHelpFormatter creates a console based on the RichHelpConfiguration as the tight coupling between the Formatter and Click's internals make it difficult to allow the Console to be configured externally (i.e. one example is that Click expects help formatting to be buffered).
  2. RichContext class was created to allow creation of the custom formatter.
  3. The Rich Command, Group, and Context now expose the Console and RichHelpConfiguration properties. I thought this might make sense, but happy to remove if it might cause problems elsewhere.
  4. Added contributor vscode settings

@BrutalSimplicity
Copy link
Contributor Author

The rich-codex workflow updated the images, but seems like the screen grabs are no longer generating colorful output for some of the commands. Not quite sure what to make of it. Maybe some leaky module config state carrying over across snapshots? When I run rich-codex or the rich-click examples I always seem to get some unsupported characters in my terminal.

image

The `command.get_usage` method generates the usage string by writing it
to a new formatter. This caused Rich to apply the styles twice.
@ewels
Copy link
Owner

ewels commented Oct 25, 2022

This is amazing, thanks so much! A tonne of stuff to go through so it may take me a few days to find time to properly look at it, but on the face of it I think it looks great 🚀

@ewels
Copy link
Owner

ewels commented Oct 25, 2022

Note - rich-codex running on: [push] is nice in principle but in practice I've been finding it super spammy on other projects. So feel free to switch it to just workflow_dispatch: if you like. Can then try to remember to run it before release.

@BrutalSimplicity
Copy link
Contributor Author

Yea, I hear you on the rich-codex workflow.

I made a fix to handle the issue I found with improper usage formatting. It looks like internally click creates a new formatter to print out the usage string, so the Rich formatting was being applied twice. Now, instead of asking click for the usage I changed that portion to call write_usage on the formatter and that seemed fix the behavior.

Other than that, I believe (please verify) that any other changes were only updates to configuration attributes and threading the formatter through to other functions.

Perhaps, it might be worth looking at moving that functionality into the formatter itself, but the PR is large enough as is, and I didn't want to accidentally break the internals.


def main(self, *args, standalone_mode: bool = True, **kwargs):
formatter = self._formatter = RichHelpFormatter(config=self.help_config)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not sure the formatter is worth exposing here. It's only needed in the exception handling at this point as the formatter used for help generation is created by click internally. I only exposed it as a possible convenience for users. Maybe the Rich console is enough.

r"(?P<metavar>\<[^\>]+\>)",
r"(?P<usage>Usage: )",
]
def _get_rich_formatter(formatter: Optional[click.HelpFormatter] = None) -> RichHelpFormatter:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is possibly not needed since the formatter entry points are in the public functions. But, I was a bit unsure of other ways users may be using the "private-by-convention" functions in this module.

My thought was maybe someone is calling these internal functions to supplement behavior, and are calling them without the newly added formatter argument. In that case, this should fall back to the original behavior since it will then resolve the formatter from the active click context or the module-level configuration.

@ewels
Copy link
Owner

ewels commented Nov 14, 2022

Hi @BrutalSimplicity,

Sorry for being slow on the review here. If I'm honest, much of what you've written is a bit beyond me 😅 I'm keen to merge, but it makes me nervous about maintaining the code if I don't really understand it. What do you think about being added as a collaborator on the repo to help out more long term? Then I would feel much happier to merge even though I'm out of my depth.

General points:

  • Screenshots in the PR are still lacking colour, even if fixed in the workflow. Can probably just do a bit of rebasing to remove those commits? I've switched to workflow_dispatch outside of this PR already.
  • Would be great to add some more docs / examples if we're adding new extensibility here.

Cheers,

Phil

@ewels ewels changed the base branch from main to pr-92 March 28, 2023 12:54
@ewels
Copy link
Owner

ewels commented Mar 28, 2023

Merging into a new branch so that I can continue on the final bits to get this merged 👍🏻

@ewels ewels changed the base branch from pr-92 to pullrequest-92 March 28, 2023 12:56
@ewels ewels merged commit 7a6f61b into ewels:pullrequest-92 Mar 28, 2023
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.

None yet

2 participants