-
-
Notifications
You must be signed in to change notification settings - Fork 661
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Include module path in virtual file names #11852
base: development
Are you sure you want to change the base?
Include module path in virtual file names #11852
Conversation
@@ -461,7 +461,7 @@ let make_macro_api ctx mctx p = | |||
in | |||
let add is_macro ctx = | |||
let mdep = Option.map_default (fun s -> TypeloadModule.load_module ~origin:MDepFromMacro ctx (parse_path s) pos) ctx.m.curmod mdep in | |||
let mnew = TypeloadModule.type_module ctx.com ctx.g ~dont_check_path:(has_native_meta) m (ctx.com.file_keys#generate_virtual ctx.com.compilation_step) [tdef,pos] pos in | |||
let mnew = TypeloadModule.type_module ctx.com ctx.g ~dont_check_path:(has_native_meta) m (ctx.com.file_keys#generate_virtual m ctx.com.compilation_step) [tdef,pos] pos in |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's rename m
to mpath
while we're here.
We could name these something like |
The |
Yes the package should be there too of course. Internally the naming doesn't really matter, the only part that's important is that the file key is unique. So a normal module path with the step and counter suffix would be fine too. |
|
RE dotpath to directory structure: Yes please. It will be nicer. I could replace dots with slashes on my dependency visualizer, but that would require special handling for |
When using
-D dump-dependencies
, macro modules show up with their file names, but for macro generated modules that's unusable (they're all named/server/file_x_y
which doesn't help much).tbh I'm not sure this is the kind of fix we need here, but I also didn't want to break things relying on current
-D dump-dependencies
output.