fmt: improve module detection when formatting imports #21134
Conversation
ttytm
force-pushed
the
fmt/improve-import
branch
from
0d78d32
to
7d20cf9
Compare
| // NOTE: A test that follows a directory strucutre and module naming like this one | ||
| // also tests whether vfmt would add a prefix to the import. | ||
|
|
||
| import a.ab |
There was a problem hiding this comment.
Why is the src. removed at all?
The less vfmt modifies the paths, the better.
There was a problem hiding this comment.
So you say to allow for both without a formatting error?
For usage of the module both paths work.
There was a problem hiding this comment.
I'd prefer the "one way" philo here when thinking about it.
There was a problem hiding this comment.
My main concern about it, is preventing oscillations, where it is removed by one run of vfmt, based on a heuristic, and then added in another run of vfmt, based on another heuristic. It seems to me safer to not modify the imports at all/not have special cases for it.
There was a problem hiding this comment.
So you say to allow for both without a formatting error?
Imho vfmt should not try to outsmart the person writing the code. It should just format the code as it is written.
There was a problem hiding this comment.
Removing the special case for the vmodules is very important though, and src. is an edge case, that will not be encountered very often, so overall, excellent work.
There was a problem hiding this comment.
Personally, I would have preferred ALWAYS stripping src.. It is supposed to be a "silent" subdir, same as "modules".
There was a problem hiding this comment.
I see the point and concerns. Yeah, since src gets some special treatment already and hopefully isn't too complex to maintain in fmt, giving developers the same code to look at accross code bases might be the right move.
There was a problem hiding this comment.
Personally, I would have preferred ALWAYS stripping
src.. It is supposed to be a "silent" subdir, same as "modules".
There are edge cases where src. is part of the import and required to be kept for the module to work. E.g. importing from a module that is structured like this issue is mentioning #19799. We might improve module lookup so that it works without it then we can always strip src..
There was a problem hiding this comment.
Improving module lookup is long overdue...
spytheman
force-pushed
the
fmt/improve-import
branch
from
7d20cf9
to
1575f60
Compare
|
(rebased over master) |
The PR improves vfmts import detection. It makes it obsolete to keep a special clause to handle modules in a
$VMODULESdirectory. So vmft now should be able can treat all modules accordingly independent of their location.It also fixes scenarios where vmft adds a
src.prefixes to imports. E.g. it would happen in any regular directory when trying to format a module like thesrc_importthat is added to the testdata in this PR. A test was added that covers two scenarios in one - removing thesrc.prefix and also not adding it an import.Running the updated test before the
fixcommit also shows other issues that are resolved with the changes.