-
-
Notifications
You must be signed in to change notification settings - Fork 273
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. Weβll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix flake8 bugbear version and fix some flake8 issues #1504
Conversation
a4402dc
to
3cebc82
Compare
Pull Request Test Coverage Report for Build 2111421131
π - Coveralls |
astroid/interpreter/objectmodel.py
Outdated
@@ -60,6 +61,26 @@ def _dunder_dict(instance, attributes): | |||
return obj | |||
|
|||
|
|||
@lru_cache(maxsize=None) | |||
def str_startswith_attr_(obj: str) -> bool: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we really cache this? Seems a bit a overkill?
astroid/interpreter/objectmodel.py
Outdated
|
||
|
||
@lru_cache(maxsize=None) | ||
def after_attr__in_str(obj: str) -> str: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here..
astroid/interpreter/objectmodel.py
Outdated
@@ -100,20 +121,16 @@ def __get__(self, instance, cls=None): | |||
def __contains__(self, name): | |||
return name in self.attributes() | |||
|
|||
@lru_cache(maxsize=None) | |||
def attributes(self): | |||
def attributes(self) -> List[str]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The expensive part of this function is (I assume) the iteration over the dir
. The other two function seem fairly cost efficient? Perhaps just remove the caching here altogether?
astroid/nodes/node_classes.py
Outdated
@@ -366,21 +366,19 @@ def get_children(self): | |||
class LookupMixIn: | |||
"""Mixin to look up a name in the right scope.""" | |||
|
|||
@lru_cache(maxsize=None) | |||
def lookup(self, name): | |||
@lru_cache(maxsize=None) # pylint: disable=cache-max-size-none # noqa |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we want a max here? 128
on the same LookupMixIn
seems like it should fit most of our uses in pylint
?
astroid/transforms.py
Outdated
|
||
from astroid.context import _invalidate_cache | ||
|
||
if TYPE_CHECKING: | ||
from astroid.nodes.node_ng import NodeNG |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know we have different opinions about this, but I like importing nodes
here as it also keeps the line much shorter and gives some consistency with the pylint
codebase. I'll leave this to you!
astroid/transforms.py
Outdated
@@ -22,18 +25,21 @@ class TransformVisitor: | |||
def __init__(self): | |||
self.transforms = collections.defaultdict(list) | |||
|
|||
@lru_cache(maxsize=TRANSFORM_MAX_CACHE_SIZE) | |||
def _transform(self, node): | |||
def _transform(self, node: "NodeNG") -> "NodeNG": |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wasn't #1242 looking at how to cache this without running into memory issues?
Maybe we can remove all the caching temporarily and review/merge #1242 ? |
I think that PR was almost good to go? |
I fixed the merge conflict when merging main on 1242, it has gone stale for a long time. What about merging the non-cache fix here first even if it fails, then #1242 ? |
a9a2929
to
ccbf43e
Compare
Let's add Edit: Ah, I misinterpreted your comment. Yeah, what you are proposing is fine as well! |
Well we won't forget about this if we merge with pre-commit failing π |
Description
Type of Changes