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

Remove dependency step in order to use the extension #21

Closed
fasfsfgs opened this issue Jan 29, 2017 · 10 comments · Fixed by #40
Closed

Remove dependency step in order to use the extension #21

fasfsfgs opened this issue Jan 29, 2017 · 10 comments · Fixed by #40

Comments

@fasfsfgs
Copy link
Contributor

coda_hale commented this in reddit

Looks like a great start, and it's awesome to see more high-quality support for Clojure!
One quick note: I'm pretty sure you can get around the manual cider-nrepl/nrepl dependencies steps if you invoke lein the same way cider does:

/usr/local/bin/lein update-in :dependencies conj \
  \[org.clojure/tools.nrepl\ \"0.2.12\"\ \:exclusions\ \
  \[org.clojure/clojure\]\] -- update-in :plugins conj \
  \[refactor-nrepl\ \"2.3.0-SNAPSHOT\"\] -- update-in :plugins conj \
  \[cider/cider-nrepl\ \"0.15.0-SNAPSHOT\"\] -- repl :headless

That also means you're not tied down to a particular version of those libraries.

@avli
Copy link
Owner

avli commented Jan 31, 2017

@fasfsfgs have you already tried this approach? One of my fellows is working on a similar stuff in this branch.

@yurtaev could it be helpful?

@avli avli changed the title remove dependency step in order to use the extension Remove dependency step in order to use the extension Jan 31, 2017
@fasfsfgs
Copy link
Contributor Author

@avli I didn't try anything yet. I'm not even sure I understand what could be done with that. =p
I'll check @yurtaev's work to see if I can be of any help later.

@avli
Copy link
Owner

avli commented Apr 11, 2017

Some time ago I implemented this feature, but it needs more polishing... The implementation is available in this branch.

@fasfsfgs
Copy link
Contributor Author

fasfsfgs commented Jul 10, 2017

@avli I'm working on your branch now.
The idea here is to remove the dependency step altogether, right?
I mean, does it make sense to continue supporting Clojure: Connect to nREPL command still?
Or are only going to provide the new clojureVSCode.startNRepl and clojureVSCode.stopNRepl?

Btw this would probably also kill #37, wouldn't it?

@avli
Copy link
Owner

avli commented Jul 10, 2017

Awesome to hear that you have taken up the initiative!

That's a good question and I don't have the answer yet. However, this particular issue says about removing the dependency step in order to use the extension. This means we may limit ourselves by doing exactly this thing – eliminating the necessity of editing profiles. The connection command, in turn, may be left untouched.

On the other hand, from the UX point of view, it would be cool if the extension would magically start providing all its goodies without any attention from a user. What isn't clear for me is what to do if I need to set up an nREPL connection explicitly? For example, I need to connect to a remote REPL.

Summing up, the solution I see at the moment is:

  1. The extension should start an nREPL instance automatically on opening a Clojure project.
  2. There must be a way to explicitly define or redefine the nREPL parameters. Maybe, we should leverage VSCode project settings in order to achieve this.

Does it make sense?

@fasfsfgs
Copy link
Contributor Author

Alright it does make sense. The need to connect to a remote REPL is a great point.

If we are going to maintain manual connection, I don't think we can rule out #31. Not sure.

I'll start by polishing those two ways of connecting to a REPL. While I do this, let's discuss here what we should provide.

I think a good start is to automatically connect to a REPL like you said. If the user has specific needs, he can disconnect from this first REPL and connect to his manual one. Or even get back to another automatic REPL.

@avli
Copy link
Owner

avli commented Jul 12, 2017

Yes, you're right, #31 is not a duplicating issue in this case.

Another idea just came out: what if we leave the "Connect to nREPL" command untouched but it won't ask for connection parameters. Instead, it will connect to an existing nREPL (if any) or start an internal one if no nREPL is running. I believe this is the approach CIDER uses. However, I'm not sure which of two is better. What do you think?

@fasfsfgs
Copy link
Contributor Author

I see. But if we don't ask for parameters, we prevent them from connecting to a remote REPL for example.

I think we should clearly distinct between the 2 connection modes. I think this will avoid confusion when using them and also simplify the logic.

Here are the commands I was thinking to provide:

  • Start REPL Automatically start and connect to a REPL. This is the main command we want to provide in this issue. This would always start a new REPL, even if the user started one himself outside the extension. If the user already started a REPL using this command, we prevent them from creating a new one (message: REPL already started). If they are connected manually to a REPL, we ask them to disconnect first.

  • Connect to a running REPL This is the today's connection mode. We continue to ask for parameters. But I was thinking about removing automatic connection in case the user has a running REPL in the same path as the project. We can always help the user fill the parameters if we find a running REPL. This would continue to keep Check if the user has declared the plugin and dependency #31 a good requirement. If they are already connected to a REPL, we ask them to disconnect first.

  • Stop/disconnect REPL We kill the REPL we started in Start REPL. If the user connected manually, we stop using the parameters informed.

When the user opens a clojure project, I think we should always do a Start REPL. If they want to manually connect to a running REPL, they'd have to disconnect first. And later, like you said, we can leverage vscode project settings to change this default behavior.

@avli
Copy link
Owner

avli commented Jul 12, 2017

@fasfsfgs Thank you for summing up the things. I'm 100% support the suggested approach.

@fasfsfgs
Copy link
Contributor Author

Great! Thanks! I'll follow those ideas then. I'll let you know if I get stuck in some case.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants