Skip to content
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

temporary_visited_module requires manually running patch_ast beforehand #171

Closed
machow opened this issue Jun 22, 2023 · 2 comments
Closed

Comments

@machow
Copy link
Contributor

machow commented Jun 22, 2023

Describe the bug

It looks like outside of using the pytest plugin and creating a loader--which both run patch_ast--running temporary_visited_module() will fail with a message like below.

AttributeError: 'Module' object has no attribute 'kind'

It looks like patch_ast mutates attributes etc.. onto ast classes that the visitor needs.

I wonder if turning the AstNode methods into function might help avoid some of the trickiness of chasing down the patching / making sure it's applied before anything that needs it? E.g. something like...

import ast

# note that lru_cache(maxsize=None) works for python < 3.9 compat
from functools import cache

@cache
def kind(self: ast.AST) -> str:    return self.__class__.__name__.lower()

kind(ast.parse("1 + 1"))

Otherwise, maybe patch_ast() could be run inside things like temporary_visited_module()?

@pawamoy
Copy link
Member

pawamoy commented Jun 22, 2023

I guess we could run patch_ast in the helper yes 🤔 It really runs only once, checking/using a global var, so there shouldn't be any perf hit.

The alternative is to tell every library user of Griffe to call it themselves, but I understand this is not ideal.

@pawamoy
Copy link
Member

pawamoy commented Jul 7, 2023

Actually @machow I tried your suggestion to use functions rather than patching AST classes. It led me to a rabbit hole of refactors and optimizations, and really paid off I think: the code is faster, uses much less memory, is better typed, and the lib is easier to use. So thanks! I'll push soon (and you'll be able to see the amount of changes if you're interested hehe), and if you could try the main branch then, to make sure there's no regressions, that would be great 🙂 (I couldn't identify any myself, through the tests suite and fuzzing on 415 installed packages). I'll notify you here!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants