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

[Fix #949] Define custom cider-default-project-type. #951

Merged
merged 1 commit into from
Jan 22, 2015

Conversation

pandeiro
Copy link
Contributor

Custom var is used with cider-jack-in when connecting to project-less REPLs.

(Both Leiningen and Boot support REPLs outside of projects: Cider users can choose which command to default to.)

@pandeiro pandeiro force-pushed the projectless-repl-bugfix branch from 3f66b39 to 7e89932 Compare January 16, 2015 13:05
@bbatsov
Copy link
Member

bbatsov commented Jan 16, 2015

Of we can simply check which command is available on the PATH. I'm not against using a defcustom, but I think the proposed name is misleading as we're talking about a situation in which there's no project.

@pandeiro
Copy link
Contributor Author

@bbatsov $PATH will probably have both lein and boot for many Clojure users, though.

What should the defcustom be called? cider-default-command maybe?

@pandeiro
Copy link
Contributor Author

cider-default-repl-command ?

@vspinu
Copy link
Contributor

vspinu commented Jan 16, 2015

As there are already cider-lein-command and cider-boot-command, there should probably be a selector like cider-prefered-boot-tool. Even better - cider-boot-tools - a list of the form '(lein boot) with tools tried in turn. This list can be easely expanded in the future. If this list is missing a tool, that tool is not even tried.

@pandeiro
Copy link
Contributor Author

@vspinu I think putting the word 'boot' into the var would be a little confusing... But I'm also not sure I see the advantage in making it a list rather than just one custom var that the user can change if necessary -- can you explain?

I've changed the var name per @bbatsov 's request and am resubmitting the PR but if need be I can rework this more.

Custom var is used with `cider-jack-in` when connecting to
project-less REPLs.
@pandeiro pandeiro force-pushed the projectless-repl-bugfix branch from 7e89932 to 73f9271 Compare January 16, 2015 16:32
@vspinu
Copy link
Contributor

vspinu commented Jan 16, 2015

@vspinu I think putting the word 'boot' into the var would be a little confusing...

If I understand correctly lein and boot are better described as boot tools and not project types (as cider-project-type might suggest). What's the right name for these automations? Build tools? Load-backends?

But I'm also not sure I see the advantage in making it a list rather than just one custom var that the user can change if necessary

The idea is that cider should go through the list of available tools and pick the first tool for which the executable is available. This way the user who has only boot on the system will not need to customize anything. The same variable gives the preferred order of completing-read/ido when the tool is requested interactively (for whatever reason). If a user removes a backend from the list, that means he/she doesn't want to see it in completions or other operations.

Having a comprehensive list of backends is a good idea in general. Your code becomes aware of all available tools and a lot of code could be written generically.

@pandeiro
Copy link
Contributor Author

@vspinu Cool, I see your point... I'm not totally sure about the implementation though, but I'll hack on it. @bbatsov you okay with this so far?

@pandeiro
Copy link
Contributor Author

@vspinu Re: terminology, yeah, "projects" are the Leiningen-inspired parlance for Clojure ... repositories. So I stuck with that. Boot might be "build", given build.boot. If there were an agnostic term to encompass both, that would be great. I personally don't think "project-type" is a misnomer -- but maybe "project-build-type" or "project-build-tool" would be better?

Regarding line 170, it is a little bit of a shortcut, since our backend-agnostic functions dispatch off of either the strings "lein" or "boot" -- the same set of values that make sense for cider-repl-command.

Anyhow thanks for the input and explanations.

@vspinu
Copy link
Contributor

vspinu commented Jan 16, 2015

Regarding line 170, it is a little bit of a shortcut, since our backend-agnostic functions dispatch off of either the strings "lein" or "boot" -- the same set of values that make sense for cider-repl-command.

Yes. I saw that, but it contains a meta assumption that actual commands == build tools names. I think the actual code is fine. The semantics is not right. I was really confused when I read it first time. The appropriate renaming will fix the semantic issue.

The boot documentation calls itself build tool. It still seems to me that in the context of cider boot tools is more appropriate, but given the confusion with the boot itself, it might be better to use build indeed. So maybe cider-build-tool instead of cider-project-type and cider-default-build-tool custom var or cider-build-tools custom list instead of cider-default-repl-command.

@bbatsov
Copy link
Member

bbatsov commented Jan 17, 2015

The boot documentation calls itself build tool. It still seems to me that in the context of cider boot tools is more appropriate, but given the confusion with the boot itself, it might be better to use build indeed. So maybe cider-build-tool instead of cider-project-type and cider-default-build-tool custom var or cider-build-tools custom list instead of cider-default-repl-command.

Projectile has long been using a terminology like lein project, maven project, etc. While I see your point, it seems to me most people like it.

@@ -98,6 +98,15 @@ version from the CIDER package or library.")
:group 'cider
:package-version '(cider . "0.9.0"))

(defcustom cider-default-repl-command
Copy link
Member

Choose a reason for hiding this comment

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

I still don't quite like this, as it's not clear this is a command used to start/run an nREPL server. Maybe the more descriptive cider-start-nrepl-server-command. The command doesn't have to be tied to lein or boot at all. Someone might like to start nREPL using java -jar ....

@bbatsov
Copy link
Member

bbatsov commented Jan 17, 2015

As I noted in a code comment - when you're outside of a lein or boot project you should be able to use any command to start an nREPL server - lein and boot are certainly not the only options.

On the other hand - anything else you're using should provide lein compatible output, which CIDER can parse, so it's not clear if many people would do something like this.

@vspinu
Copy link
Contributor

vspinu commented Jan 19, 2015

As I noted in a code comment - when you're outside of a lein or boot project you should be able to use any command to start an nREPL server - lein and boot are certainly not the only options.

This is probably true inside lein/boot projects as well. One might want to have a custom startup commands without altering cider-lein-command or cider-boot-command in order to have multiple alternatives at startup.

This is why cider-boot-backends list is a good idea. It standardizes the infrastructure. If someone wants to add a new command or tool, it has to add a symbol to that list, say joot, then add cider-joot-command, cider-joot-present-p and cider-joot-parameters and he is done. On startup from outside of the lein/boot project you will be prompted interactively for the tool as cider will be aware of all available tools.

The current API could be rewritten generically. For example:

(defun cider-command-present-p (build-tool)
  (let ((fun (intern (format "cider-%s-present-p" build-tool))))
    (when (fboundp fun)
      (funcall fun))))

BTW, cider-command-present-p would probably be better as cider-program-present-p.

@triclops200
Copy link

I would like to mention that right now cider is broken for files not in projects, and this would fix it. It cannot start a server to connect to, as cider-project-type returns nil.

bbatsov added a commit that referenced this pull request Jan 22, 2015
[Fix #949] Define custom cider-default-project-type.
@bbatsov bbatsov merged commit cf507d2 into clojure-emacs:master Jan 22, 2015
@bbatsov
Copy link
Member

bbatsov commented Jan 22, 2015

I'll merge this to spare users of the MELPA package the pain of dealing with the existing bug, but this is something that we can definitely improve upon.

@pandeiro
Copy link
Contributor Author

@bbatsov I think that's prudent; a more robust, future-proof solution would best be tackled by someone else for now.

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.

4 participants