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

[Draft] Fix typing and other bugs #126

Merged
merged 12 commits into from
Oct 7, 2023
Merged

[Draft] Fix typing and other bugs #126

merged 12 commits into from
Oct 7, 2023

Conversation

dwreeves
Copy link
Collaborator

@dwreeves dwreeves commented Oct 6, 2023

WOW. This was a lot of work. I am exhausted.

Long story short: I wanted to get the typing in rich-click to work with minimal hacking. I'm not 100% done (I think I may want this working in strict mode). But in the process, I ended up uncovering a lot of weird little issues:

  • The typing never actually worked and mypy wasn't really working to its full potential. The reason why is due to the pre-commit config, which used language: python and had no third party dependencies. I swap over to language: system in the pre-commit and now everything is being actually tested for real.
  • The full @command, @command(), @command("name") etc. API was not being supported. See the test cases now for test_rich_config_decorator_order-- I have added every single way I can think of to turn a callbacks into a RichCommand (or subclass thereof, i.e. RichGroup). Some things I uncovered here:
    • One big problem here was that the context stuff was being resolved in the rich_click.command and rich_click.group decorators. This meant something like click.Group(...).command(cls=RichCommand) did not actually use the rich context correctly! That realization is what motivated adding all those pytest.param()s i.e. ways of turning callbacks into RichCommands.
      • The solution to this was to move parsing the context settings to the __init__ for RichCommand.
      • Oh, and when messing around with that, I discovered the decorator wasn't working properly. It was extracting help_config even though the key that was being set elsewhere was rich_help_config. 😬
    • Also, I don't actually think this test case ever worked to begin with? We end up asserting a non-markdown --help output!
  • We want to override typing for pass_context because otherwise the type annotation ctx: RichContext does not work in a function signature, due to the fact that the typing is contravariant. I opened an issue in actual click about it here: Typing for click.decorators.pass_context is not covariant and does not support click.Context subclasses pallets/click#2623 But my conclusion is they should not fix it / they should mark as "won't do" or something. I think it is better for us to just fix it on our end.
    • Also, it turns out that in the case of from click import *, the type annotations are not properly overridden according to mypy! I am not sure what that is about. I manually imported everything in click (notwithstanding anything being deprecated), and add added a __getattr__ to future proof.
      • The __init__.py will require more work. It is not done! I need to revisit this and it's one of the bigger reasons why this PR is a "draft."
  • I learned that Click 9.0 is actually deprecating the MultiCommand and BaseCommand classes. 😬 So that recent addition is getting slightly rolled back.

I'm definitely missing some notes. So many changes in this one! I am going to need to sit on this. Also, I don't expect as I submit this for the Click 7.x tests to pass; haven't gotten to that yet. But Click 8.x tests do pass.

@dwreeves dwreeves requested a review from ewels October 6, 2023 21:10
@dwreeves
Copy link
Collaborator Author

dwreeves commented Oct 6, 2023

Some manual testing. Note to self: there is still one more case to handle:

import rich_click as click


@click.group("foo")
@click.rich_config(help_config=click.RichHelpConfiguration(use_markdown=True, max_width=60))
@click.option("--x")
@click.pass_context
def foo(ctx: click.Context, x: str) -> None:
    """This uses **markdown**"""
    pass

@foo.command
@click.pass_context
def bar(ctx: click.RichContext) -> None:
    """Hello, _world!_"""

foo()

In this case, the rich_config works for the base command, but is not passed into the subcommand. This doesn't seem like that should be the correct behavior:

>>> python foo.py --help    
                                                            
 Usage: foo.py [OPTIONS] COMMAND [ARGS]...                  
                                                            
 This uses markdown                                         
                                                            
╭─ Options ────────────────────────────────────────────────╮
│ --x       TEXT                                           │
│ --help          Show this message and exit.              │
╰──────────────────────────────────────────────────────────╯
╭─ Commands ───────────────────────────────────────────────╮
│ bar  Hello, world!                                       │
╰──────────────────────────────────────────────────────────╯

>>> python foo.py bar --help
                                                                                                                                                                             
 Usage: foo.py bar [OPTIONS]                                                                                                                                                 
                                                                                                                                                                             
 Hello, _world!_                                                                                                                                                             
                                                                                                                                                                             
╭─ Options ─────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╮
│ --help      Show this message and exit.                                                                                                                                   │
╰───────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────────╯

The good news? The typing works in strict mode! 😏

>>> mypy -m foo --strict
Success: no issues found in 1 source file

@dwreeves dwreeves force-pushed the fix-typing branch 3 times, most recently from ec11aed to 1a4affe Compare October 6, 2023 22:25
@dwreeves
Copy link
Collaborator Author

dwreeves commented Oct 6, 2023

Yay, all tests passing! I am super excited about this update and we're close to the finish line. 😄😄😄

Last three steps, in no particular order:

  • Revisit the approach to __init__.py. It's possible there is a better solution than what I am currently doing. I have not experimented enough.
  • Consider getting mypy --strict mode working. I don't see why we shouldn't. The click package itself also uses strict mode, and we're copying a ton of stuff from them. A lot of our downstream users may also be using strict mode and supporting their code rather than being a nuisance for them would be great. (I detest --strict mode personally and avoid it, but that's for when I am doing my own personal projects; I feel it's a necessary evil when building a tool for thousands of users.)
  • rich_config inside of RichGroups should pass to objects created by RichGroup.command and RichGroup.group, when said objects do not have their own rich_config. Also, create test case asserting as much.

@dwreeves
Copy link
Collaborator Author

dwreeves commented Oct 7, 2023

I'm feeling comfortable merging this at this point.

There just isn't a good way around the __init__.py issue. I just need to be careful with the latest version of Click in the works. I just double checked all the highest-level API changes that are planned for Click 9, and we should be good for now.

Mypy strict mode is working and turned on. Very excited about that.

I'm feeling very exhausted and don't want to add a test case just yet for the parent context inheritance thing. I left it open as an issue: #127.

OK, at this point I think I need to just do some real world integration testing. I may also want to consider tackling any other tiny open issues before releasing 1.7.0dev to PyPI. But this PR finalizes all the code changes I had planned in advance to make this week.

@dwreeves dwreeves merged commit a39f762 into ewels:main Oct 7, 2023
32 checks passed
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

1 participant