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

Use Code Insiders - #30

Closed
orenc17 wants to merge 4 commits into
coder:masterfrom
orenc17:master
Closed

Use Code Insiders#30
orenc17 wants to merge 4 commits into
coder:masterfrom
orenc17:master

Conversation

@orenc17

@orenc17 orenc17 commented Apr 22, 2019

Copy link
Copy Markdown

Add a flag for using extensions and settings from VSCode Insiders

@scsmithr scsmithr left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Everything else looks good.

Comment thread main.go Outdated
@foresthoffman

Copy link
Copy Markdown

@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

coadler commented Apr 22, 2019

Copy link
Copy Markdown
Contributor

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

@ammario

ammario commented Apr 22, 2019

Copy link
Copy Markdown
Member

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

@coadler

coadler commented Apr 22, 2019

Copy link
Copy Markdown
Contributor

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

@ammario

ammario commented Apr 22, 2019

Copy link
Copy Markdown
Member

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

SscSPs commented Apr 22, 2019

Copy link
Copy Markdown
Contributor

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
Copy Markdown

@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

coadler commented Apr 22, 2019

Copy link
Copy Markdown
Contributor

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

@SscSPs

SscSPs commented Apr 22, 2019

Copy link
Copy Markdown
Contributor

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

ammario commented Apr 22, 2019

Copy link
Copy Markdown
Member

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 Apr 22, 2019
@orenc17

orenc17 commented Apr 22, 2019

Copy link
Copy Markdown
Author

is it better now?

@ammario

ammario commented Apr 22, 2019

Copy link
Copy Markdown
Member

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

@orenc17

orenc17 commented Apr 22, 2019

Copy link
Copy Markdown
Author

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

ammario commented Apr 22, 2019

Copy link
Copy Markdown
Member

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

@orenc17

orenc17 commented Apr 22, 2019

Copy link
Copy Markdown
Author

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

@orenc17

orenc17 commented Apr 22, 2019

Copy link
Copy Markdown
Author

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

6 participants