metals-vscode icon indicating copy to clipboard operation
metals-vscode copied to clipboard

refactor: Do not use .metals for checking the metals version

Open tanishiking opened this issue 2 years ago • 3 comments

This PR replaces $PROJECT_DIR/.metals/versions-meta.json and $HOME/.metals/versions-meta.json" to workspaceState and globalState respectively.

I remember someone has trouble because Metals creates .metals in their home directory (on Discord, but I couldn't find the link to the discussion). Anyway, it's best practice to use context.globalState (or workspaceState) for managing the state of extension, and it makes easy to test (in the future) 👍

Let me test locally for a while before merging.

tanishiking avatar Jul 25 '22 09:07 tanishiking

@tanishiking I initially wanted to store it in inside settings but vscode doesn't allow writing into unknown fields. Every setting must be declared in package.json. There is an option to rename global directory from .metals to smth else

dos65 avatar Jul 25 '22 10:07 dos65

I initially wanted to store it in inside settings but vscode doesn't allow writing into unknown fields. Every setting must be declared in package.json.

Are you talking about the coursier mirror settings? https://github.com/scalameta/metals-vscode/blob/bfc4de113c688338d6776a1e330d4ddedd742c09/src/mirrors.ts#L28-L31 https://scalameta.org/metals/docs/troubleshooting/proxy/ I don't have a plan to replace it with something else at least in this PR.

I don't have a context why metals-vscode has settings to edit the coursier mirror properties, but I'm wondering why users want to configure it via metals-vscode instead of setting global coursier mirrors properties by themselves.

tanishiking avatar Jul 26 '22 05:07 tanishiking

I don't have a context why metals-vscode has settings to edit the coursier mirror properties, but I'm wondering why users want to configure it via metals-vscode instead of setting global coursier mirrors properties by themselves.

It's just a bit more convenient for the users to have it metals instead of manually searching how to do it.

tgodzik avatar Jul 26 '22 08:07 tgodzik

It's just a bit more convenient for the users to have it metals instead of manually searching how to do it.

Ok, it would be awesome if we could completely remove the usage of ~/.metals directory for metals-vscode, but we can work on that in another issue / PR :+1: (I personally think, vscode settings shouldn't change the global behavior (coursier mirror setting in this case). I think people won't expect a change in the vscode setting create/modify the global coursier setting though...)

tanishiking avatar Sep 01 '22 08:09 tanishiking

Also added unit test (while refactoring the project structure) for needCheckForUpdate https://github.com/tanishiking/metals-vscode/pull/82 I'll open a PR against this repository after this PR has merged.

tanishiking avatar Sep 01 '22 08:09 tanishiking

(I personally think, vscode settings shouldn't change the global behavior (coursier mirror setting in this case). I think people won't expect a change in the vscode setting create/modify the global coursier setting though...)

I understand what you mean, but it was really complicated for people to find the right place for where to put the file and it is far from obvious. Maybe it would be better to have mirror as a parameter in that case, but I think last time I checked that was not possible. And this was hard to get working right :sweat_smile:

tgodzik avatar Sep 01 '22 09:09 tgodzik

it was really complicated for people to find the right place for where to put the file and it is far from obvious. Maybe it would be better to have mirror as a parameter in that case, but I think last time I checked that was not possible. And this was hard to get working right 😅

I see, thank you for sharing the background! Maybe we can keep those settings without placing proxy files in ~/.metals directory, I'll take a look!

tanishiking avatar Sep 01 '22 11:09 tanishiking