Fix get_decorator_name with callable in between

[Llandy3d]

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

#283

Checklist:

• I have updated the documentation in the README.md file or my changes don't require an update.
• I have added an entry in CHANGELOG.md.
• I have added or adapted tests to cover my changes.
• I have run tox -e fix-style to format my code and checked the result with tox -e style.

[codecov-commenter]

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%)	⬆️

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

[Llandy3d]

the only thing problematic for older versions is the test, so we ignore this specific test

[Llandy3d]

in the regression test this will return something in the lines of @hist.labels.time I'm not completely sure if this is what is wanted, but atleast now in that specific case it's not failing 🤔

[RJ722]

The other option would be to return calls with args/kwargs preserved (like @hist.labels("labelvalue").time).

For context, the reason why get_decorator_method exists is for us to be able to support the --ignore-decorators flag. Preserving args/kwargs here would make it possible for users to ignore for particular arguments/kwargs only (e.g. --ignore-decorators "@hist.labels("labelA").time"), but I'm not sure it's possible to do that statically. Besides, even if it is, it would require significant changes to the code and I don't think it worth the maintenance effort. If a user wants to ignore for one value of labelValue, they probably want to do so for other values too.

[RJ722]

Thanks for the PR, @Llandy3d!

Other than the few minor comments, I think we should mention this behavior (how to specify a decorator with a callable in between to --ignore-decorators) in the docs as well. Perhaps, add an example in the README?

[RJ722]

Let's rename this to test_get_decorator_callable_in_between? We can also drop the from pometheus... import and hist definition since we don't really need it for testing this part.

[RJ722]

Let's add another test case here? It also tests for all decorated types.

@pytest.mark.skipif(
    sys.version_info < (3, 9), reason="requires python3.9 or higher"
)
@pytest.mark.parametrize(
    "decorated",
    [
        ("def foo():"),
        ("async def foo():"),
        ("class Foo:"),
    ],
)
def test_get_decorator_name_multiple_callable(decorated):
    decorated = f"{decorated}\n\tpass"
    code = f"""\
@foo
@bar.prop
@z.func("hi").bar().k.foo
@k("hello").doo("world").x
@k.hello("world")
{decorated}
"""
    check_decorator_names(
        code,
        ["@foo", "@bar.prop", "@z.func.bar.k.foo", "@k.doo.x", "@k.hello"],
    )

[jendrikseipp]

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

[Llandy3d]

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

[RJ722]

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]

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

[jendrikseipp]

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:

• utils.get_decorator_name() preserves its old behaviour but returns the empty string for decorator names that it cannot parse (caught in an except AttributeError block)
• we update the readme file to briefly reflect that we only support decorators of the form @foo.bar(x, y, z)

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

[jendrikseipp]

@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]

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]

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