Verify nodes' str() and repr() don't raise errors/warnings #2198
Conversation
|
I changed the test flavor to pytest and fixed all |
|
Also something weird is going on with the PR, since it is trying to reapply a commit already found in the main branch (same hash and everything), I guess it has to do with me not having some sort of permission to update the PR or execute a certain flow. |
|
Could you rebase or merge main in your branch ? |
It should be. My fork shows my branch being just one commit ahead and none behind. 😮 |
crazybolillo
force-pushed
the
1881-crazybolillo
branch
3 times, most recently
from
5ac8540 to
5fa419b
Compare
Codecov Report
Additional details and impacted files@@ Coverage Diff @@
## main #2198 +/- ##
==========================================
+ Coverage 92.68% 92.71% +0.02%
==========================================
Files 94 94
Lines 10830 10830
==========================================
+ Hits 10038 10041 +3
+ Misses 792 789 -3
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| if name == "self": | ||
| continue | ||
|
|
||
| if "int" in param_type.annotation: |
There was a problem hiding this comment.
I like this trick, never thought of doing it like this!
| @@ -1350,7 +1350,7 @@ def fromlineno(self) -> int: | |||
| # lineno is the line number of the first decorator, we want the def | |||
| # statement lineno. Similar to 'ClassDef.fromlineno' | |||
| lineno = self.lineno or 0 | |||
| if self.decorators is not None: | |||
| if getattr(self, "decorators", None) is not None: | |||
There was a problem hiding this comment.
I'd prefer just setting decorators in the __init__. Since it can be None anyway this doesn't interfere with the typing of the attribute.
astroid/nodes/node_ng.py
Outdated
| @@ -246,7 +246,7 @@ def accept(self, visitor): | |||
| def get_children(self) -> Iterator[NodeNG]: | |||
| """Get the child nodes below this node.""" | |||
| for field in self._astroid_fields: | |||
| attr = getattr(self, field) | |||
| attr = getattr(self, field, None) | |||
There was a problem hiding this comment.
For which of the nodes is this necessary? get_children isn't only used in __str__ so I'm worried people might depend on this returning an AttributeError.
astroid/nodes/node_classes.py
Outdated
| @@ -859,33 +859,33 @@ def find_argname(self, argname, rec=False): | |||
| return None, None | |||
|
|
|||
| def get_children(self): | |||
There was a problem hiding this comment.
See other comment. If we could find a way where we only impact __str__ and __repr__ I would be very much in favour of that.
crazybolillo
force-pushed
the
1881-crazybolillo
branch
2 times, most recently
from
9b6dd82 to
f556237
Compare
|
Ah I see, makes sense to only touch The error is the following: I solved inside Thanks for the review 🤝 |
Calling str() or repr() on certain nodes fails either with errors or warnings. A unittest was added to verify this behaviour and find the offending nodes. Code has been corrected, mainly by accesing node's attributes safely and using placeholders if necessary. Closes pylint-dev#1881
crazybolillo
force-pushed
the
1881-crazybolillo
branch
from
f556237 to
f5028e2
Compare
|
Thanks to you guys for the insightful comments, I also prefer the list comprehension, I changed that and added the missing comment 🤝 |
Calling str() or repr() on certain nodes fails either with errors or warnings. This commit adds unittests to find and reproduce this behavior with the endgoal of fixing it.
Closes #1881
Type of Changes