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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

馃悰 Fix context_settings for a Typer app with a single command #210

Merged
merged 7 commits into from Jul 2, 2022

Conversation

daddycocoaman
Copy link
Contributor

Addresses #208

When the design choice is to create an typer.Typer() object with context_settings, that context should be passed even when only a single command is registered. This is already handled when there are more than two commands since a Group is created in that scenario but should also exist for single commands.

@codecov
Copy link

codecov bot commented Dec 18, 2020

Codecov Report

Merging #210 (e8f947d) into master (e663db0) will not change coverage.
The diff coverage is 100.00%.

@@            Coverage Diff            @@
##            master      #210   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files          252       252           
  Lines         5287      5296    +9     
=========================================
+ Hits          5287      5296    +9     
Impacted Files Coverage 螖
tests/test_others.py 100.00% <100.00%> (酶)
typer/main.py 100.00% <100.00%> (酶)

Continue to review full report at Codecov.

Legend - Click here to learn more
螖 = absolute <relative> (impact), 酶 = not affected, ? = missing data
Powered by Codecov. Last update e663db0...e8f947d. Read the comment docs.

@malthunayan
Copy link

@daddycocoaman great work on your PR, maybe your predicate in line 241:

if not single_command.context_settings and typer_instance.info.context_settings:
    ...

could be:

if isinstance(single_command.context_settings, DefaultPlaceholder) and not isinstance(typer_instance.info.context_settings, DefaultPlaceholder):
    ...

That way if a user (for some odd reason) sets the context settings explicitly to None in their decorator it would no longer use the context settings instantiated in the typer instance. Also, I think it aligns more with how the codebase determines if a change is required. I'm not entirely sure, what do you think?

@daddycocoaman
Copy link
Contributor Author

daddycocoaman commented Dec 18, 2020

That makes perfect sense. When I first tried using DefaultPlaceholder, I must have messed up the conditional because the local test failed. I'll make that change.

@daddycocoaman
Copy link
Contributor Author

daddycocoaman commented Dec 18, 2020

@malthunayan It turns out that CommandInfo and Typer.command don't use the DefaultPlaceholder class to represent default values, which is why it didn't work when I originally tried it. I tried adding it to the classes, but it broke some other things further along, like documentation generation.

doc = <typer.models.DefaultPlaceholder object at 0x00000297B400C7F0>

    def cleandoc(doc):
        """Clean up indentation from docstrings.

        Any whitespace that can be uniformly removed from the second line
        onwards is removed."""
        try:
>           lines = doc.expandtabs().split('\n')
E           AttributeError: 'DefaultPlaceholder' object has no attribute 'expandtabs'

doc        = <typer.models.DefaultPlaceholder object at 0x00000297B400C7F0>

..\..\..\AppData\Local\Programs\Python\Python38\lib\inspect.py:631: AttributeError

Since the value is already set to None, looks like it shouldn't be an issue. I'd rather leave that much of a change up to @tiangolo (although I would be willing to do it) so for now I'll just use
if not single_command.context_settings and not isinstance(typer_instance.info.context_settings, DefaultPlaceholder) to align with existing code and keep the PR simple. Although, I do think those parameters should eventually become DefaultPlaceholder.

@malthunayan
Copy link

Hey, @daddycocoaman,
Thank you very much on your detailed explanation. I didn't actually try out the predicate I've suggested above, and I do agree with everything you have said.

@davised
Copy link

davised commented Mar 17, 2021

I was just running into this issue and I'm glad this PR is here. @tiangolo do you plan on pulling this in?

@github-actions
Copy link

github-actions bot commented Jul 2, 2022

馃摑 Docs preview for commit e8f947d at: https://62c07354f22cef5e2cb4216f--typertiangolo.netlify.app

@tiangolo tiangolo changed the title fix context settings for single command 馃悰 Fix context_settings for a Typer app with a single command Jul 2, 2022
@tiangolo tiangolo merged commit 778d5bd into tiangolo:master Jul 2, 2022
@tiangolo
Copy link
Owner

tiangolo commented Jul 2, 2022

Awesome, thanks a lot for your contribution @daddycocoaman! 馃嵃

And thanks for the discussion here everyone! 馃憦 馃檱

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

4 participants