justarandomgeek / vscode-factoriomod-debug

Factorio Mod Tool Kit
Other
100 stars 24 forks source link

Rewrite clusterio module requires #121

Closed Laar closed 3 months ago

Laar commented 3 months ago

Clusterio modules use a different convention module paths. This rewrites those paths so that the requires are correctly resolved.

Danielv123 commented 3 months ago

Would love to have this, although I guess it could cause issues for people who have a folder called modules/xxx in their mod.

justarandomgeek commented 3 months ago

@JanSharp any thoughts on this? seems okay to me probably / maybe a separate toggle if that's easy? or a way for people to specify their own custom rules here for other cases?

Laar commented 3 months ago

Forced it to only match paths starting with the pattern so that it is less likely to trigger unwanted. Separate toggle might indeed be good.

Danielv123 commented 3 months ago

For an idea of how useful this would be, my local development install has 278 such requires in 127 lua files. Getting proper type annotations would be super nice.

JanSharp commented 3 months ago

I'm trying to think about how custom rules could be structured to be able to do this specific thing and it would be weird due to that exclusion of "modules/clustorio". Best I could think of is

"Lua.runtime.pluginArgs": [
  "--require-path-keep", "^modules/clusterio/",
  "--require-path-gsub", "^modules/[^/]-/", "module/",
]

Which is probably decent... but it really doesn't feel clean. But maybe. The plugin needs a proper arg parser anyway so it seems like now's a good time to do it. If I end up being too lazy it'll likely end up being just --clustorio-modules.

Edit: oh, note that a backslash doesn't work in require/module paths anyway. It's either forward slashes or dots

justarandomgeek commented 3 months ago

yeah if generalized custom rules is too crazy it's fine if we end up skipping that for now, it just seemed like an important question to think about in the process

JanSharp commented 3 months ago

Quick question to double check, is it really supposed to replace it with module/ and not modules/? The comment and the code both say module/ so I'd assume this is in fact intentional, but just wanted to make sure.

JanSharp commented 3 months ago

The code and docs match, and if you say it's working then nothing to change :+1:

Laar commented 3 months ago

Quick question to double check, is it really supposed to replace it with module/ and not modules/? The comment and the code both say module/ so I'd assume this is in fact intentional, but just wanted to make sure.

Yes that is correct (initially read your comment wrong). The final save contains modules as path, but the plugin uses the singular module as directory

justarandomgeek commented 3 months ago

sorry for the delay - this is included in 1.1.44 which is out now:

"Lua.runtime.pluginArgs": [
        "--clusterio-modules",
    ],