Implement "clean" command

[alessioprestileo]

Implement "clean" command, which reads the output directory from the user settings and deletes it.

[wclr]

Implement "clean" command, which reads the output directory from the user settings and deletes it.

Can you explain the motivation for this and the usage scenario?

[alessioprestileo]

@wclr I'm a beginner in PureScript, and a few days ago I struggled because the vscode-ide-purescript extension was behaving weirdly on the project I'm working on. It turned out the solution was to remove the output dir and rebuild the project, so I thought it would be useful to have such a command integrated in the extension. The reason why the vscode-ide-purescript extension was behaving weirdly turned out to be that I had updated the purescript package from 0.13.8 to 0.14.1, and then opened vscode on a project that I had built before, so it was built using purescript@0.13.8. The mismatch between the version the project was built with and the version that was running in the extension caused the extension to not recognize any imports and not recognize any types except for the primitive types.

[wclr]

Right, I'm aware too of such a problem. I would consider this to be a compiler responsibility to handle this case of updated version correctly, just to free a user of the manual hassle.

Though I'm ok with this feature. But I would be more specific about the visible command name e.g., "Clean compiled output".

[nwolverson]

@wclr ultimately "output needs cleaned to build properly" in the case of version upgrades should be sorted by the compiler, and "stale output requires build for IDE" could be triggered by an IDE prompt.

There are a couple of other cases where I would usually nuke output. Sometimes things go weird switching back and forth on git branches - maybe that's also something also that could be fixed. But when moving or removing PureScript modules, the compiled results stay in output/ by the design of the compiler

Anyway I don't care about providing one more command that many people might not find useful - main concern is if this is dangerous and could delete data unintentionally

[wclr]

@nwolverson

Anyway I don't care about providing one more command that many people might not find useful

I prefer to care about such things as well. And though I've said that I'm ok with the feature if to think more carefully providing such command conveys that this is a "normal scenario" for work, though it is actually not, as it is just a workaround for compiler's shortcoming and just UI shortcut for terminal rm -rf output.

I would prefer LS itself to be more intelligent about it if possible.

main concern is if this is dangerous and could delete data unintentionally

Cleaning up stuff is always potentially dangerous, so this is also the point.

@alessioprestileo do you manage to reproduce the issue on the simple example?

---

But when moving or removing PureScript modules, the compiled results stay in output/ by the design of the compiler

I thought about this issue, and probably the compiler is not going to be aware of and intelligent about such things, but probably LS could if it could handle cache-db.json content with hashes and clean up the output directory selectively, as well do more intelligent fast rebuilds.

[nwolverson]

I thought about this issue, and probably the compiler is not going to be aware of and intelligent about such things, but probably LS could if it could handle cache-db.json content with hashes and clean up the output directory selectively, as well do more intelligent fast rebuilds.

This is 100% not the job of the language server, the correctness of incremental and "fast rebuild" compilation is a compiler concern. At best the language server is providing an appropriate context.

[wclr]

This is 100% not the job of the language server, the correctness of incremental and "fast rebuild" compilation is a compiler concern. At best the language server is providing an appropriate context.

Right, but the compiler doesn't do currently some things it should do. For example, the compiler does fast rebuilds even if the source didn't change, or it can not handle module removal/rename. This could be handled on the LS side, so LS could be the point where the current compiler's shortcomings could be temporarily compensated to enhance user experience, as it takes more time to introduce changes to the compiler. I may be wrong about it, but I prefer an issue to be solved even if it assumes some temporary workaround (that really helps to improve the experience).

On the OP problem I did some testing, it seems it's easy to reproduce, and if there is a version mismatch compiler does emit the error:

[Error] Version mismatch for the externs at: d:\Code\elm2purs\output/Unsafe.Coerce\externs.cbor Expected: 0.14.1 Found: 0.14.0

that could be handled.

But cleaning the output does not always solve the issue when the wrong config is done. Example:

If one wants to use node_modules located purs one should add:

"purescript.addNpmPath": true,
"purescript.buildCommand": "npx spago build --purs-args --json-errors",

Otherwise, if you don't set purescript.buildCommand here spago will use globally installed purs, and will get version mismatch as well. I think this is too config problem that should be consistently fixed in some way.

[nwolverson]

Otherwise, if you don't set purescript.buildCommand here spago will use globally installed purs, and will get version mismatch as well

As far as I know addNpmPath is true, then spago is invoked with the local npm bin in PATH, which it should then just pick up. This is something that always just worked before. Does npx complicate this somehow?

Edit: If something isn't working right here can you raise an issue, at the very least the config can be documented

[wclr]

As far as I know addNpmPath is true, then spago is invoked with the local npm bin in PATH, which it should then just pick up. This is something that always just worked before. Does npx complicate this somehow?

I see this in the code that is provides updated PATH env var, but I tell what I see in my tests. Found it: https://github.com/nwolverson/purescript-language-server/issues/133

[alessioprestileo]

@wclr @nwolverson I only have these 2 arguments for introducing the feature:

1. I don't understand how this command could be dangerous, since it deletes a temporary folder which is only used for the build output
2. I have never seen a tool where you can build but you cannot clean the build output, so I thought by adding this command the vscode-ide-purescript would look more like other building tools I have used in the past

[wclr]

I have never seen a tool where you can build but you cannot clean the build output, so I thought by adding this command the vscode-ide-purescript would look more like other building tools I have used in the past

I think this is a valid argument, but I have a suggestion here: for safety and other reasons this should not remove everything in the output directory but cache-db.json and directories that contain externs.cbor.

The other reasons may include as an example: I considered for myself to have package.json/node_modules in the output directory (to treat as a workspace package), this obviously not a standard scenario, but why not. An this will be definitely safer.

[alessioprestileo]

It sounds good to me 👍 @nwolverson what do you think?

[nwolverson]

@alessioprestileo that sounds good to me, that should guard against bad things happening if e.g. output is misconfigured to something weird

[alessioprestileo]

@wclr @nwolverson I made the improvements suggested by @wclr, feedback is very welcome :-)

I am logging to the console output every file and every dir that were removed... Do you think it's too much? It seems like useful information.

[alessioprestileo]

I added an info log message at the start and finish of the clean command, and I stopped logging messages about removal of directory content when the whole dir was removed.

[nwolverson]

Logging is good. I think we're good to go then?

[alessioprestileo]

Yep, if there's nothing you want me to change then it's done 👍