Fix get_decorator_name with callable in between #285
Conversation
Codecov Report
@@ Coverage Diff @@
## main #285 +/- ##
=======================================
Coverage 99.38% 99.38%
=======================================
Files 19 19
Lines 647 650 +3
=======================================
+ Hits 643 646 +3
Misses 4 4
Help us with your feedback. Take ten seconds to tell us how you rate us. |
|
|
||
| @pytest.mark.skipif( | ||
| sys.version_info < (3, 9), reason="requires python3.9 or higher" | ||
| ) |
There was a problem hiding this comment.
the only thing problematic for older versions is the test, so we ignore this specific test
| parts.append(decorator.attr) | ||
| decorator = decorator.value | ||
| if not isinstance(decorator, ast.Call): | ||
| break |
There was a problem hiding this comment.
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 🤔
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
tests/test_utils.py
Outdated
| @pytest.mark.skipif( | ||
| sys.version_info < (3, 9), reason="requires python3.9 or higher" | ||
| ) | ||
| def test_get_decorator_name_regression(): |
There was a problem hiding this comment.
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.
| def myfunc(): | ||
| pass | ||
| """ | ||
| check_decorator_names(code, ["@hist.labels.time"]) |
There was a problem hiding this comment.
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"],
)
Llandy3d
changed the title
|
@jendrikseipp just pinging as I'm not sure if github gives notifications for emoji responses, but seems like a positive one? 😁 |
|
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! |
|
Hi @RJ722 , I probably misunderstood the hooray icon response as a positive review 😅 |
|
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 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? |
|
sorry I missed the first message. TIL about that on decorators :) |
|
I'm making the fix discussed above in #284. |
Description
get_decorator_namewould fail with anAttributeErroron decorators on the style of theprometheus_clientwhere 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.Calland can build the full name.Related Issue
#283
Checklist:
tox -e fix-styleto format my code and checked the result withtox -e style.