jazzband / pip-tools

A set of tools to keep your pinned Python dependencies fresh.
https://pip-tools.rtfd.io
BSD 3-Clause "New" or "Revised" License
7.67k stars 608 forks source link

Absolute imports over relative imports #2024

Closed jamesbraza closed 9 months ago

jamesbraza commented 9 months ago

Trying to test __main__ code per this comment, it seems the best way is to use importlib as instructed here.

However, importlib.machinery doesn't play nice with relative imports. So this PR moves the internals to use absolute imports.

Contributor checklist
Maintainer checklist
jamesbraza commented 9 months ago

Out of curiosity, why do you say absolute imports are an inconvenient hack?

In my own style, I find absolute imports easier to follow, and PEP 8 is a proponent of absolute imports too:

Absolute imports are recommended, as they are usually more readable and tend to be better behaved

PEP 8 used to discourage relative imports more aggressively (source) too:

Relative imports for intra-package imports are highly discouraged. Always use the absolute package path for all imports.

I've also hit situations where relative imports can cause problems, depending on how one invokes python and configures PYTHONPATH. From my local testing tonight, absolute imports work with importlib.machinery, whereas relative imports don't.


Anyways, feel free to close this out if pip-tools's style prefers relative imports, my need for this PR is no more because an alternate approach was pointed out in the source PR

webknjaz commented 9 months ago

It's not currently enforced, so is probably up to each person. https://mail.python.org/pipermail/python-dev/2010-October/104476.html suggests that in the times of Python 2.5, things were less stable/obvious maybe, more complicated. So I'm not surprised that people realized that it wasn't relevant anymore. Plus, Python 3 changed a lot of stuff for the better.

I've also hit situations where relative imports can cause problems, depending on how one invokes python and configures PYTHONPATH. From my local testing tonight, absolute imports work with importlib.machinery, whereas relative imports don't.

That's exactly why I don't want absolute imports: it encourages people to keep misconfiguring their environments, unfortunately. I don't think that it's a tolerable side effect.

We actually have an intentional relative import in ansible-core that is also a sanity check that we haven't broken our import machinery level hooks. A few months ago, colleagues from another team tripped over that and also proposed a hack that was removing that relative import, which of course was rejected. I helped them realize that they were calling the machinery incorrectly, and that caused an actual fix to be found. Apparently, some people miss/forget that there's a package argument in the APIs of importlib, and it's meant to be used. I have a hunch that if you pass what the package is to that API, then it'll magically work for you.

webknjaz commented 9 months ago

@jamesbraza FWIW, I looked into that other PR and realized that what you suggest would probably mess with the import machinery. So I would still use subprocesses to test it.