Closed jdb78 closed 3 years ago
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 from
-imports, and I had already reviewed that just before this one came in, so to remain polite, I merged it first.
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 sys.path
), and use the -m
option instead, to run the file as a module. Doing this from the top level of the project source tree, everything works as expected.
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 .
level to point to the directory containing the module being analyzed, because that's what python -m
does.
Think it should be doable. I hopefully will have some time in the next 4 weeks and will give it a go.
We can now resolve from . import foo
, etc. Also, I started with some very simple tests that can be extended. @Technologicat, do you want to have a look?
@Technologicat, could have a look at the changes?
@Technologicat, could have a look at the changes?
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 pyan
, I've invited you as a collaborator to this repo.
@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_z
andfrom mod_x import mod_y
. Relative imports such asfrom ..mod_x import mod_y
are 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.