Support attributes in varname and nameof #14
Conversation
|
pylint says def _node_name(node):
if isinstance(node, ast.Name):
return node.id
elif isinstance(node, ast.Attribute):
return node.attr
else:
raise VarnameRetrievingError(
f"Can only get name of a variable or attribute, "
f"not {ast.dump(node)}"
)Personally I think it looks nice and consistent when the clauses are short - easy to mentally parse quickly. Should I change it? pylint is pretty fussy, it's brave to put that in CI. |
|
It is... but just want to keep some uniformed style if there are more PRs coming in. You can simply ignore the pattern: def _node_name(node):
# pylint: disable=no-else-return
if isinstance(node, ast.Name):
return node.id
elif isinstance(node, ast.Attribute):
return node.attr
else:
raise VarnameRetrievingError(
f"Can only get name of a variable or attribute, "
f"not {ast.dump(node)}"
) |
|
Do you like the general idea of the PR? I probably need more tests before merging. |
I love it! |
|
In the process of doing this I've noticed some problems with how nameof works. I see that if executing doesn't give you the actual node you use the statement, and then you use def test_original_nameof():
x = 1
y = []
assert len(y) != original_nameof(x) == 'x'it finds the call to The reason def test_bird():
assert original_nameof(nameof) == 'nameof'
import dis
dis.dis(test_bird)Here's the output from Obviously it's very complicated and I don't know what exactly it's doing but for example at the beginning it calculates
The bytecode version is also affected by this. I suggest that:
|
|
Good catch. I am wondering if we can do a strict check for such a situation and make it work. For example, we tell people that it doesn't work if there are more than 2 items to compare ( |
|
Well, even for 2-item comparisons, I am guessing we are still failing this: with pytest. I have added a test, which proves my guessing: def test_both():
a = 1
b = 1
> assert varname_module.nameof(a) != varname_module.nameof(b)
tests/test_varname.py:479:
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _
args = (1,), result = 'a', @py_assert3 = 2, @py_assert5 = 'b'
def nameof(*args):
"""Test both implementations at the same time"""
result = original_nameof(*args, caller=2)
if len(args) == 1:
> assert result == _bytecode_nameof(caller=2)
E AssertionError: assert 'a' == 'b'
E - b
E + a
tests/test_varname.py:22: AssertionErrorFailing on def test_both():
a = 1
b = 1
> assert original_nameof(a) != original_nameof(b)
E AssertionError: assert 'a' != 'a'
E + where 'a' = original_nameof(1)
E + and 'a' = original_nameof(1)
tests/test_varname.py:479: AssertionError |
|
I agree with all your suggestions. Even though we could make it work somehow when there is one |
|
It's quite easy to check that there's exactly one My worry is users being confused by code working fine one moment and then not working when they make a change that seems innocent. I think it's better to just find out as soon as possible that |
|
We should just probably drop support for such a case when |
|
Yes, it sounds like we agree. |
|
Yes. Do you want to do another PR to drop the support first, and then switch back to this one? |
|
I think I'll make another PR first. |
For example
nameof(self.x) == 'x'. I think this is a pretty intuitive enhancement. The bytecode version already does this.