Relative imports should work even if they are not within the project #1684
Conversation
There was a problem hiding this comment.
Looks like this improves some cases, though to be honest I'm not sure I completely follow how this code works (I've only had a fairly quick look).
I'll have another play with #1655 soon (which I think we're going to want the str/Path fixing from anyway) and have another look at this afterwards.
| if not self.import_path or not self._infer_possible: | ||
| if not self.import_path: | ||
| if self._fixed_sys_path: | ||
| # This is a bit of a special case, that maybe should be |
There was a problem hiding this comment.
The description here of why the code is needed/is special feels out of step with the test which validates the behaviour. Perhaps I'm missing something, but is there more to this code than just the niche case described here?
There was a problem hiding this comment.
No it's simply that.
It's just a special case, because it breaks the basic idea of something being on the sys path. Do you need further explanations?
There was a problem hiding this comment.
Ah sorry. I think if this code were here just for the special case, which this comment suggests is to do with the same name appearing at several levels in the hierarchy, then on reading this comment alone I would assume that it would only be activated by that special case and that the test to validate it would exercise that case. However the test doesn't actually seem to hit that case (the directory names it chooses differ at each level), yet this code path is taken.
Clearly the test that is present is a useful test and this codepath is useful in making it work, so perhaps this comment could be tweaked to clarify that this codepath is here for all cases of namespace packages, rather than just for the case of matching names?
There was a problem hiding this comment.
I guess my comment just wasn't good enough. Because having the same names is just one case. Will try to improve :)
There was a problem hiding this comment.
I added a bit to the comment.
Once I merge this, I will probably make a release.
Also sorry about the delay, but had to await my firstborn :)
Co-authored-by: Peter Law <PeterJCLaw@gmail.com>
Co-authored-by: Peter Law <PeterJCLaw@gmail.com>
Codecov Report
@@ Coverage Diff @@
## master #1684 +/- ##
=======================================
Coverage 94.52% 94.53%
=======================================
Files 79 79
Lines 11762 11780 +18
=======================================
+ Hits 11118 11136 +18
Misses 644 644
Continue to review full report at Codecov.
|
|
@PeterJCLaw I merged, because I think this was at least good enough. Please let me know if you think it's still not good enough and I will fix it for the next release. |
See comments in code.
This is another potential solution for #1655 (and an answer to #1662 as well).
@PeterJCLaw I was a bit confused in the beginning why this was happening, but I understand it better now. This was an issue on all platforms. We could of course still use #1662 or at least parts of it, but I guess this is the more important part.