Resolve from imports
#31
Conversation
|
Hi! Thanks for the PR! This is definitely a nice addition to Pyan. However, there was an older PR for adding an alternative implementation for analyzing The code you suggested looks very nice, though, so maybe we could try to have our cake and eat it too. There's just one thing I'd like to change. I'd like to support relative imports. Partly the reason I admit is selfish - I use them extensively in my own library code, e.g. in unpythonic - but even generally speaking, some codebases do use them, so Pyan would be able to analyze a larger set of codebases if it supported them than it can if it does not. Concerning bad practice, hasn't that changed again? In the pre-Python3 days, PEP8 used to ban relative imports outright. In 2013, Guido said that the recommendation was outdated, because [in Python 3] relative imports look different from absolute imports and so the two can no longer be confused. On the other hand, Nick Coghlan replied in the same thread that it's still nice to avoid relative imports, but those who know what they are doing are free to violate that guideline. Nowadays, PEP8 states that explicit relative imports are an acceptable alternative to absolute imports. So going by PEP8 as it stands in 2020, I think we should support them. Personally, I think relative imports have their pros and cons. It's nice to be able to run an in-development copy of a library right in the source tree, while being able to rely on that it's not importing some installed copy from somewhere else. (This in itself has its pros and cons. It could be considered a bad practice with the rise of ubiquitous virtualenvs and CI. On the other hand, anything that adds steps to the edit/run cycle hinders the development UX.) One just has to be careful, though, to never execute a script from inside a package directly (to avoid the package directory from ending up on So, what do you think, could we make the suggested changes work with relative imports, too? I think it's safe to assume (at least if we document it in the README) that no one who uses relative imports runs bare scripts from such codebases, anyway - so, we can assume the |
|
Think it should be doable. I hopefully will have some time in the next 4 weeks and will give it a go. |
jdb78
force-pushed
the
feature/resolve_from_imports2
branch
from
367c42f
to
3539bb6
Compare
|
We can now resolve |
|
@Technologicat, could have a look at the changes? |
pyan/analyzer.py
Outdated
| @@ -469,52 +544,48 @@ def visit_ImportFrom(self, node): | |||
| # resolve relative imports correctly. The current "here's a set of files, | |||
| # analyze them" approach doesn't cut it. | |||
| # | |||
| # As a solution, we register imports here and later, when all files have been parsed, resolve them. | |||
| # | |||
| # relative imports are currently not supported, i.e. `from ..mod import xy` is not correctly resolved | |||
There was a problem hiding this comment.
This comment out of date? (Relative imports now handled below.)
pyan/analyzer.py
Outdated
| from_node.defined = True | ||
| if to_node is None or to_node in self.defines_edges[from_node]: | ||
| status = status or False |
There was a problem hiding this comment.
Line 1508 does nothing, regardless of the old value of status?
There was a problem hiding this comment.
indeed. deleted the line
| @@ -1656,7 +1717,7 @@ def collapse_inner(self): | |||
| # What about incoming uses edges? E.g. consider a lambda that is saved | |||
| # in an instance variable, then used elsewhere. How do we want the | |||
| # graph to look like in that case? | |||
|
|
|||
| # BUG: resolve relative imports causes (RuntimeError: dictionary changed size during iteration) | |||
There was a problem hiding this comment.
Do you know if this is still the case? (Just curious, since this PR changes the implementation of that feature.)
There was a problem hiding this comment.
We actually now can resolve those incoming uses edges
There was a problem hiding this comment.
Ah, I meant the BUG: resolve relative imports causes.... Should probably have included a line number there. :)
The question was, do you know if we still need to for name in list(self.nodes) instead of just for name in self.nodes?
In any case the code is fine like that, it's just the comment that would be nice to fix if it's no longer up to date. But I think we can merge this PR already - that can be fixed later.
| @@ -1,5 +1,6 @@ | |||
| #!/usr/bin/env python3 | |||
| # -*- coding: utf-8 -*- | |||
| from typing import Union, List | |||
There was a problem hiding this comment.
Eh, please disregard this particular comment, this is needed due to #42.
Yes, of course. Sorry for the long delay - as usual, I've been working on another project (this time, an advanced macro expander for Python). Review posted. Mostly looks good to me. Having some tests is a really nice addition. When you have time, could you look at the final minor points in the review? I think after those this is ready for merging. |
|
Fixed a merge conflict that arose due to merging in one of the small PRs that were posted this autumn. Kept your version of the code. |
|
@jdb78 : As you've posted some high-quality PRs to |
|
@Technologicat modified the comments - could you have another look? |
|
Looks good to me. Merged. Thanks! |
This PR addresses import such as
from mod_x import func_y as func_zandfrom mod_x import mod_y. Relative imports such asfrom ..mod_x import mod_yare not supported. However, relative imports are considered bad style and discouraged. It's probably not too much work to add this functionality.As the package structure has to be known to resolve imports, this is done as a post-processing step relinking the modules.