jendrikseipp / vulture

Find dead Python code
MIT License
3.38k stars 148 forks source link

Fix get_decorator_name with callable in between #285

Closed Llandy3d closed 1 year ago

Llandy3d commented 2 years ago

Description

get_decorator_name would fail with an AttributeError on decorators on the style of the prometheus_client where you actually have a call in between instead of just at the end of it. Ex. @hist.labels('label').time()

This pr changes the method to loop so that we will catch all the ast.Call and can build the full name.

Related Issue

https://github.com/jendrikseipp/vulture/issues/283

Checklist:

codecov-commenter commented 2 years ago

Codecov Report

Merging #285 (5417b4a) into main (fb126c7) will increase coverage by 0.00%. The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #285   +/-   ##
=======================================
  Coverage   99.38%   99.38%           
=======================================
  Files          19       19           
  Lines         647      650    +3     
=======================================
+ Hits          643      646    +3     
  Misses          4        4           
Impacted Files Coverage Δ
vulture/utils.py 98.66% <100.00%> (+0.05%) :arrow_up:

Help us with your feedback. Take ten seconds to tell us how you rate us.

jendrikseipp commented 2 years ago

Thanks for making these additional changes @Llandy3d ! I'll have a look at the PR when @RJ722 is happy :-)

Llandy3d commented 2 years ago

@jendrikseipp just pinging as I'm not sure if github gives notifications for emoji responses, but seems like a positive one? 😁

RJ722 commented 2 years ago

Hi @Llandy3d! I appreciate the reminder -- It comes at just the right time, when I have found some time to work on open source. I'll give another round of review in a few hours. ^>^. Thanks for the patience!

Llandy3d commented 2 years ago

Hi @RJ722 , I probably misunderstood the hooray icon response as a positive review 😅 No rush at all, and thank you!

jendrikseipp commented 1 year ago

First off, thanks for looking into this, @Llandy3d and @RJ722 ! The code fixes the issue you were seeing, but I think we need a more general solution to cover the flexible decorators that Python 3.9 allows: a decorator can be any expression now (https://peps.python.org/pep-0614/). For example @foo[2] is valid, but not accepted by the code in this PR.

Since we cannot hope to cover all possible decorator names, I suggest we use the following basic solution:

@Llandy3d do you want to incorporate these changes into this PR?

jendrikseipp commented 1 year ago

@Llandy3d If you don't have the time to continue the PR that's completely fine of course. If so, maybe @RJ722 or I can pick up where you left off.

Llandy3d commented 1 year ago

sorry I missed the first message. TIL about that on decorators :) it makes sense, I don't know if/when I would be able to pick this back up so please feel free to work on it! 🙇‍♂️

jendrikseipp commented 1 year ago

I'm making the fix discussed above in #284.