Skip to content
This repository has been archived by the owner on Jan 17, 2021. It is now read-only.

Use Code Insiders #30

Closed
wants to merge 4 commits into from
Closed

Use Code Insiders #30

wants to merge 4 commits into from

Conversation

orenc17
Copy link

@orenc17 orenc17 commented Apr 22, 2019

Add a flag for using extensions and settings from VSCode Insiders

Copy link
Contributor

@scsmithr scsmithr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Everything else looks good.

main.go Outdated Show resolved Hide resolved
@foresthoffman
Copy link

@scsmithr Would it make sense to also have an extensions-dir and user-data-dir flag, since both of those are available on code-server?

https://github.com/codercom/code-server/blob/5f40ebb8457380aa1e9955302d9e41a6b042ef14/packages/server/src/cli.ts#L23-L24

@coadler
Copy link
Contributor

coadler commented Apr 22, 2019

That seems sensible but this PR is fine on its own. Should make an issue

@ammario
Copy link
Member

ammario commented Apr 22, 2019

Thank you for the PR, but #34 is easily implemented and will categorically solve this problem.

@coadler
Copy link
Contributor

coadler commented Apr 22, 2019

Wouldn't it make sense to have some sane defaults? These two would probably cover most use cases

@ammario
Copy link
Member

ammario commented Apr 22, 2019

It's unclear which path to use when both exist. I have VS Code insiders installed right now but I don't actually use it, so this change would silently break sshcode for me.

@SscSPs
Copy link
Contributor

SscSPs commented Apr 22, 2019

I think @ammario 's concern can be solved if we keep the selected version to be used in a config file may be.
Should be a good way of handling multiple different version

@foresthoffman
Copy link

@SscSPs I'm wary of requiring an additional file just to specify where the configuration dir is. That sounds a bit like a catch 22. Using environment variables, or at the very least CLI flags, would be much cleaner and wouldn't deviate from the standard configuration of code-server or Code.

@coadler
Copy link
Contributor

coadler commented Apr 22, 2019

We could potentially prompt users if both are found, and remember on consecutive runs

@SscSPs
Copy link
Contributor

SscSPs commented Apr 22, 2019

Imho doing that would be too much cli interaction when our goal is to provide a GUI text editor over SSH...
I think that the people looking for applications like this are the ones who don't like much CLIs, otherwise they would be good with vim/Emacs/nano

@ammario
Copy link
Member

ammario commented Apr 22, 2019

Agreed with @foresthoffman that a configuration file is plain overkill.

A selection prompt doesn't feel very unixey to me. Will open a PR for the environment variable solution.

@orenc17 orenc17 force-pushed the master branch 2 times, most recently from 762aec5 to bf1da12 Compare April 22, 2019 18:36
@orenc17 orenc17 changed the title Use Code Insiders config dir if exists Use Code Insiders Apr 22, 2019
@orenc17
Copy link
Author

orenc17 commented Apr 22, 2019

is it better now?

@ammario
Copy link
Member

ammario commented Apr 22, 2019

@orenc17 how do feel about the solution in #39 instead of this?

@orenc17
Copy link
Author

orenc17 commented Apr 22, 2019

You can integrate this PR into #39
When the environment variable isn't set you should revert to default which will be chosen using a flag

@ammario
Copy link
Member

ammario commented Apr 22, 2019

Don't you think that having two ways of doing the same thing is bad UX?

@orenc17
Copy link
Author

orenc17 commented Apr 22, 2019

IMHO the environment variables should be used only if vscode/vscode-insiders is not installed in the default path

@orenc17
Copy link
Author

orenc17 commented Apr 22, 2019

closing this to make way for #40

@orenc17 orenc17 closed this Apr 22, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants