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

New metric: dynamic complexity #393

Open
sobolevn opened this issue Dec 12, 2018 · 11 comments
Open

New metric: dynamic complexity #393

sobolevn opened this issue Dec 12, 2018 · 11 comments
Assignees
Labels
level:advanced Needs a lot of care rule request Adding a new rule

Comments

@sobolevn
Copy link
Member

sobolevn commented Dec 12, 2018

Rule request

Thesis

Original idea belongs to: Tin Markovic.

We need to detect functions that use too much dynamic features of python or so called "magic".
So, it is pretty much the same as McCabe complexity but for dynamic structures.
We just count uses of these structures and if it tops the threshold then we raise a violation.

What do we count as "magic"? Here's the list to be extended:

  1. *args and **kwargs in function parameters
  2. getattr, setattr, hasattr functions
  3. all direct magic methods: .__setattr__, .__div__, etc
  4. all magic constants: __name__, __file__, etc
  5. @property
  6. * and ** for argument expansion
  7. decorators, despite being tracked individually
  8. catching AttributeError, KeyError, IndexError

We can also try to implement the same metric for classes. This way we can control:

  1. metaclasses
  2. __slots__ and other magic fields
  3. magic methods
  4. decorators, despite being tracked individually

Reasoning

We need to track thing that might fail. We need to be extra careful with magic, since it is hard to use.

Status

This is an early draft. Please, feel free to suggest any ideas about this topic.

@sobolevn sobolevn added question Further information is requested rule request Adding a new rule labels Dec 12, 2018
@sobolevn
Copy link
Member Author

Guys, I would love to hear your opinions / suggestions on this topic: @proofit404 @malinoff @orsinium

@ivlevdenis
Copy link

ivlevdenis commented Dec 12, 2018

decorator

@sobolevn
Copy link
Member Author

We also can check .get for dictionaries. That's quite the same as getattr.

@orsinium
Copy link
Collaborator

orsinium commented Dec 12, 2018

3 point isn't so moot as other, because all these methods have much better way to use it. You can write a / b instead of a.__div__(b) or type(a) instead of a.__class__. I think, it will be better to move this point to another issue.

@orsinium
Copy link
Collaborator

We also can check .get for dictionaries. That's quite the same as getattr .

This:

result += a.get(b, 1)

Or this:

try:
  result += a[b]
except KeyError:
  result += 1

First one is faster and shorter.

@sobolevn
Copy link
Member Author

@orsinium considering your example: thanks, I have added it to the list. We should also track some magic exceptions.

@proofit404
Copy link
Contributor

I think we can deny *args and **kwargs everywhere except decorator definition.

def decorator_name(f):

    def wrapper(*args, **kwargs): # <- this is fine, decorator can be applied to any function
        return f(*args, **kwargs)

    return wrapper

@proofit404
Copy link
Contributor

I'm not sure about metaclasses. Is this related to the Django model or Injector from the dependencies? Probably metaclasses are fine if they come from third party library.

@sobolevn
Copy link
Member Author

sobolevn commented Dec 12, 2018

Hold on, guys, I was not clear enough. The purpose of this metric is not to deny, but to count occurrences of the given examples.

So, it is pretty much the same as McCabe complexity. We just count uses of these structures and if it tops the threshold then we raise a violation. I have copied this explanation to the issue's body.

@sobolevn sobolevn added this to the Version 0.10.0 milestone Jun 20, 2019
@sobolevn sobolevn removed this from the Version 0.10.0 milestone Jul 10, 2019
@sobolevn sobolevn added this to the Version 0.13 milestone Oct 7, 2019
@sobolevn sobolevn added dependencies Pull requests that update a dependency file level:advanced Needs a lot of care and removed question Further information is requested level:advanced Needs a lot of care dependencies Pull requests that update a dependency file labels Oct 7, 2019
@sobolevn sobolevn self-assigned this Oct 7, 2019
@sobolevn sobolevn removed this from the Version 0.13 milestone Nov 12, 2019
@uhbif19
Copy link
Contributor

uhbif19 commented Nov 20, 2019

@sobolevn It is not clear to me, what is "dynamic" or "magic" in decorators and properties.

For me "dynamic" as opposite to static, must be something that can increase probability of runtime fails or make their error messages less clear. For example, using getattr(obj, attr_name) may lead no opaque or unexpected errors if attr_name is comes from somewhere else.

There is no possibility for such effects in properties. Using property will produce errors in same situations as in plain method. Same with decorators.

@sobolevn
Copy link
Member Author

Yes, I agree with @uhbif19

I have spent multiple hours thinking and implementing this feature. It is not ready yet, but I hope to release it one day.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
level:advanced Needs a lot of care rule request Adding a new rule
Projects
None yet
Development

No branches or pull requests

5 participants