Skip to content
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

use globalStorageUri for user settings #1295

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

Stunkymonkey
Copy link

fix for #1294

this might be a breaking change!?

@Stunkymonkey
Copy link
Author

on NixOS I already submitted a patch for this: NixOS/nixpkgs@a00e878
So this is already tested by me and others.

@LasaleFamine
Copy link
Member

LasaleFamine commented Feb 29, 2024

Hi @Stunkymonkey thanks for the PR.
I think this indeed would be a breaking change since this.userConfigFileUri is used to load and write the user configuration, the old one won't be migrate with only this change.
You can see the references:
https://github.com/Stunkymonkey/vsc-material-theme/blob/use-globalStorageUri-for-settings/src/core/extension-manager.ts#L121
https://github.com/Stunkymonkey/vsc-material-theme/blob/use-globalStorageUri-for-settings/src/core/extension-manager.ts#L91

I think it would be good to check if the old one is present and if so switch the location to the new one, and the check can be removed when we release a major version. cc @equinusocio

@Stunkymonkey
Copy link
Author

Stunkymonkey commented Feb 29, 2024

@LasaleFamine I have no development setup locally, so I am coding blind... without testing it. Be warned.

So I added some more to check for the old file and if existent use it, else use the new one.

Is this what you had in mind?

PS: there might be await missing. not sure.

@LasaleFamine
Copy link
Member

@Stunkymonkey actually in this way we are considering the old file as something to use still. If the objective is to migrate to the new use config location, we should copy the old file if present in the new location.
If the configuration can work either in both locations maybe this change can be ok.

@Stunkymonkey
Copy link
Author

Stunkymonkey commented Mar 5, 2024

@LasaleFamine so you prefer this new solution? Or do you want a combination of both?

@LasaleFamine
Copy link
Member

LasaleFamine commented Mar 7, 2024

@Stunkymonkey thanks for the change. This feels to me the correct way to change the config file. Did you have the possibility to test it?

@Stunkymonkey
Copy link
Author

@Stunkymonkey thanks for the change. This feels to me the correct way to change the config file. Did you have the possibility to test it?

Sadly no. I do not even have npm installed. Is it possible you go on from here?

@equinusocio equinusocio requested review from equinusocio and removed request for equinusocio March 8, 2024 17:56
@equinusocio
Copy link
Member

Please setup the whole environment to work and debug this extension. There are many typescript errors.

@Stunkymonkey Stunkymonkey force-pushed the use-globalStorageUri-for-settings branch from 6462966 to 6aed5d0 Compare April 8, 2024 21:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants