Submodule refactor #108833
Submodule refactor #108833
Conversation
The module system already uses the parent modules _type as a fallback, so we don't need to inject the file in a weird way
infinisil
force-pushed
the
submodule-refactor
branch
from
9afde8f to
e1aa9a8
Compare
ofborg
bot
added
10.rebuild-darwin: 0
| else value | ||
| if isAttrs value && shorthandOnlyDefinesConfig | ||
| then { _file = file; config = value; } | ||
| else { _file = file; imports = [ value ]; } |
There was a problem hiding this comment.
nis now unused.- Usually an extra
importsindirection doesn't matter, but some error messages print a path through the import graph. Those will be a bit harder to understand after this change
So it doesn't seem to be purely refactoring.
In all fairness those messages aren't great, but this change is a small step back.
Perhaps we could revisit those error messages and make them report an imports "trace" with actual file/line/column positions.
There was a problem hiding this comment.
Boy was I wrong.
This seems to be a solution to the same problem that #177157 solves.
nis now unused.
n was a technicality of a previous solution for adding _file. Nothing more, nothing less.
Usually an extra imports indirection doesn't matter, but some error messages print a path through the import graph.
I don't think these occur frequently, and if they do they're a problem that can be solved separately. Adding more _file, like this code does, may prevent such messages in the first place.
off-topic section 🐢
So it doesn't seem to be purely refactoring.
Picking nits...
Perhaps we could revisit those error messages and make them report an imports "trace" with actual file/line/column positions.
Something with unsafeGetAttrPos seems like a better path for improving location reporting.
|
I marked this as stale due to inactivity. → More info |
stale
bot
added
the
2.status: stale
stale
bot
removed
the
2.status: stale
| coerce = unify: value: if isFunction value | ||
| then setFunctionArgs (args: unify (value args)) (functionArgs value) | ||
| else unify (if shorthandOnlyDefinesConfig then { config = value; } else value); | ||
|
|
||
| allModules = defs: modules ++ imap1 (n: { value, file }: |
There was a problem hiding this comment.
| allModules = defs: modules ++ imap1 (n: { value, file }: | |
| allModules = defs: modules ++ map ({ value, file }: |
Originally these commits were included in #98952, but they are independent of that PR. Just a test and a small refactoring for more clarity (and less reliance on module internals). Ping @roberth