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

python3.9: forbid anything, but dotted names inside decorators #1241

Closed
sobolevn opened this issue Mar 6, 2020 · 5 comments · Fixed by #1858
Closed

python3.9: forbid anything, but dotted names inside decorators #1241

sobolevn opened this issue Mar 6, 2020 · 5 comments · Fixed by #1858
Labels
Hacktoberfest Hactoberfest fun! help wanted Extra attention is needed level:starter Good for newcomers rule request Adding a new rule

Comments

@sobolevn
Copy link
Member

sobolevn commented Mar 6, 2020

Rule request

Thesis

After python3.9 we would be able to use this as a decorator:

@decorator_generator[start[5]:end_generator(start[5], end[-1])](gen=DecEnum.generate, **options).build() / overloaded_operator()
def some(): 
    ...

WHAT?! WHY?!

We need to add a rule that forbids it.
Only names, attributes, and calls should be allowed after @

Reasoning

This is an extra complexity that is totally not required. I don't have any valid use-cases for this.
Use variables, functions, attributes. It is not that hard!

Related: https://www.python.org/dev/peps/pep-0614/

@sobolevn sobolevn added the rule request Adding a new rule label Mar 6, 2020
@sobolevn sobolevn added this to the python3.9 milestone Mar 7, 2020
@lewisacidic
Copy link

I would argue the PEP does provide a good example use case:

@buttons[0].clicked.connect
def spam():
    ...

vs having to do

button_0 = buttons[0]

@button_0.clicked.connect
def spam():
    ...

The second one looks confusing and hacky in my opinion, and I'm glad the PEP was accepted.

That said, I agree complex decorator expressions would destroy readability. Perhaps requiring a low complexity for decorator expressions might be a more measured solution than a blanket ban?

@sobolevn
Copy link
Member Author

sobolevn commented Mar 9, 2020

@lewisacidic I am glad that you brought this up!

Let me explain the problem in this PEP example.

When you write @buttons[0].clicked.connect you are really making several very important assumptions.

First, buttons[0] is really special. Because it is the only one that is connected to this function. So, it should have a name! And a clear one. Not button_0. Let's say that it is a registration-related button. So, @registration_button.clicked.connect is way more readable @buttons[0].clicked.connect

We can also use the alternative approach:

def spam():
    ...

buttons[0].clicked.connect(spam)
# or even `for button in buttons: button.clicked.connect(spam)`

That's why we won't allow this syntax. It is broken by the very core idea.

@lewisacidic
Copy link

I agree it is not a fantastic example, the variable name button_0 is bad, and button[0].anything smells. But what if you have a dictionary of buttons:

@buttons["registration"].clicked.connect
def spam():
     ...

I do think index notation is more succinct and doesn't compromise readability in some contexts.

In any case, thanks for clarifying the reasoning behind this issue: it's a matter of enforcing explicit variable naming, rather than avoiding messy expressions as decorators which was my interpretation from the initial comment. I'll respectfully probably disable this rule as and when it lands.

Off topic, but thanks for your work on this library!

@ruwaid4
Copy link
Contributor

ruwaid4 commented Apr 2, 2020

Hello, could i take this one?

@sobolevn
Copy link
Member Author

sobolevn commented Apr 2, 2020

Nope, sorry. python3.9 is not even released. Let's wait 🙂

@sobolevn sobolevn added Hacktoberfest Hactoberfest fun! help wanted Extra attention is needed level:starter Good for newcomers labels Oct 6, 2020
@sobolevn sobolevn modified the milestones: python3.9, Version 0.15 aka New runtime Oct 23, 2020
sobolevn added a commit that referenced this issue Feb 8, 2021
@sobolevn sobolevn mentioned this issue Feb 8, 2021
sobolevn added a commit that referenced this issue Feb 9, 2021
* Closes #1241

* Fixes CI

* Fixes CI

* Fixes CI

* Fixes CI

* Fixes CI

* Fixes CI

* Fixes CI

* Fixes CI

* Fixes CI
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Hacktoberfest Hactoberfest fun! help wanted Extra attention is needed level:starter Good for newcomers rule request Adding a new rule
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants