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

Add support for optional decorator parentheses #1896

Merged
merged 1 commit into from
Feb 19, 2022

Conversation

angus-lherrou
Copy link
Contributor

@angus-lherrou angus-lherrou commented May 14, 2021

Checklist:

  • Add tests that demonstrate the correct behavior of the change. Tests should fail without the change.
  • Add or update relevant docs, in the docs folder and in code.
  • Add an entry in CHANGES.rst summarizing the change and linking to the issue.
  • Add .. versionchanged:: entries in any relevant code docs.
  • Run pre-commit hooks and fix any issues.
  • Run pytest and tox, no tests failed.

@angus-lherrou
Copy link
Contributor Author

Maintainers, I'm not sure if this change needs any docs updates?

@ThiefMaster
Copy link
Member

Wouldn't it be better if the decorators were smart enough to work even without parentheses, ie if there's exactly one positional argument and that argument is a callable, assume it's the decorated function...

@davidism
Copy link
Member

davidism commented May 14, 2021

I go back and forth on that pattern. Right now I'm on the side of "there should be one way to do things". Even if we can detect it, better to tell everyone to do something consistently. I'd have to look at the mypy docs, but I have a feeling the "decorator or factory" pattern makes typing more complicated too.

For example, non-factory decorators don't typically also support the factory pattern. (Although maybe they should, then PyCharm incorrectly doing @pass_context() wouldn't cause an error either...)

@ThiefMaster
Copy link
Member

I know pytest does it for example, but no idea if they use type hints..

@ThiefMaster
Copy link
Member

@davidism
Copy link
Member

@angus-lherrou if you're up for it, want to implement this dual option instead of showing an error? If not, we can merge this then make another issue about supporting plain decorators.

@angus-lherrou
Copy link
Contributor Author

My first thought was actually the same, but I spent some time trying to implement the optional parentheses and it just wasn't working the way I was hoping it would. I can take a look at how pytest implements it and take another crack at it.

@davidism davidism added this to the 8.1.0 milestone May 14, 2021
@angus-lherrou angus-lherrou changed the title Add descriptive error messages for when parentheses are omitted on decorators Add support for optional decorator parentheses May 14, 2021
@angus-lherrou
Copy link
Contributor Author

@davidism I figured it out! When I tried before I didn't realize that I needed to call the inner function before returning it if the decorator isn't called at the invocation site.

@angus-lherrou
Copy link
Contributor Author

I suppose it would be good to add this new capability in the documentation somewhere

@angus-lherrou angus-lherrou mentioned this pull request Oct 13, 2021
@davidism davidism merged commit 24992ce into pallets:main Feb 19, 2022
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Mar 6, 2022
@angus-lherrou angus-lherrou deleted the 1359-uncalled-decorators branch March 29, 2022 15:17
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

raise a more descriptive exception when @command decorator isn't called
3 participants