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

Load all files (clj, cljc, & cljs) on cider-load-all-files. #2558

Merged
merged 2 commits into from
Jan 7, 2019
Merged

Load all files (clj, cljc, & cljs) on cider-load-all-files. #2558

merged 2 commits into from
Jan 7, 2019

Conversation

rduplain
Copy link
Contributor

@rduplain rduplain commented Jan 5, 2019

#1983 adds cider-load-all-files; the current pull request touches it up to include cljc files in addition to clj files.

There's a larger discussion on ClojureScript repls, especially since loading cljc will eval in all active cljs repls. But cljc is needed in order to load "all files" in a Clojure repl. Before the current pull request, cider-load-all-files is incomplete for clj repls. If this proposed change does not have consensus, then let's discuss options to cider-load-all-files or variations for clj/clj+cljc/cljs+cljc.


Before submitting the PR make sure the following things have been done (and denote this
by checking the relevant checkboxes):

  • The commits are consistent with our contribution guidelines
  • [N/A] You've added tests (if possible) to cover your change(s).
  • All tests are passing (make test)
  • All code passes the linter (make lint) which is based on elisp-lint and includes
  • You've updated the changelog (if adding/changing user-visible functionality)
  • You've updated the user manual (if adding/changing user-visible functionality)

Thanks!

If you're just starting out to hack on CIDER you might find this section of its
manual
extremely useful.

@bbatsov
Copy link
Member

bbatsov commented Jan 5, 2019

There's a larger discussion on ClojureScript repls, especially since loading cljc will eval in all active cljs repls. But cljc is needed in order to load "all files" in a Clojure repl. Before the current pull request, cider-load-all-files is incomplete for clj repls. If this proposed change does not have consensus, then let's discuss options to cider-load-all-files or variations for clj/clj+cljc/cljs+cljc.

Shouldn't what we load here be based on the current REPL or file context? I guess running the command would surprise someone running it from a cljs repl or a cljs file.

I don't get the part about cljc code getting evaluated in all cljs repls. That definitely doesn't sound like the right or desired behaviour to me. I recall in the past we were doing some magic to load cljc files in both clj and cljs repls, but given those were often out of sync I think we agreed that it doesn't make sense to automatically try to load cljc files in this manner.

"All" means clj, cljc, and cljs.
@rduplain
Copy link
Contributor Author

rduplain commented Jan 5, 2019

Thanks for the fast reply. My primary concern was that cider-load-all-files has a bug, as below, but let's find a complete solution.

topic: cider-load-all-files

#1983 states the intent behind cider-load-all-files:

Useful when the nrepl host is on another machine, and you wish to "force" the state of your local file system onto that remote nrepl all at once.

In order to accomplish this (on a clj repl), CIDER needs to load cljc files in addition to clj files. Before this pull request, cider-load-all-files does not work as documented, which is a bug.

For context in this discussion, cider-load-all-files is bound to C-c C-M-l.

topic: active cljs repls

There's a larger discussion on ClojureScript repls, especially since loading cljc will eval in all active cljs repls.

The initial commit (338146e) on this pull request is a fix for the original intent of cider-load-all-files. With this patch, cider-load-all-files will "just work" on clj repls in CIDER. But if CIDER is loading into a cljs repl, then the "all-files" part of the function name is a lie (hence the update to the doc in this pull request, to name clj/cljc explicitly), because cider-load-all-files makes no attempt to load cljs files.

conclusion: expand cider-load-all-files to "do the right thing"

The doc for cider-load-file says:

The heavy lifting is done by cider-load-buffer.

Indeed. The cider-load-buffer function is iterating all active repls and loading the buffer into each. The necessary APIs are already in place to load clj, cljc, and cljs as part of "all-files" just by updating the regular expression:

(mapcar #'cider-load-file
          (directory-files-recursively directory "\\.clj[cs]?$")))

The only missing piece in doing this: a user error is reported for every file which does not have a matching repl, cljs files when there is no active cljs repl. That's a simple change in cider-map-repls.

New commit incoming ...

@rduplain rduplain changed the title Load clj & cljc files on cider-load-all-files. Load all files (clj, cljc, & cljs) on cider-load-all-files. Jan 5, 2019
@rduplain
Copy link
Contributor Author

rduplain commented Jan 5, 2019

I tested 75a683f manually with cider-jack-in and cider-jack-in-clj&cljs.
cider-load-all-files does the following:

  • loads clj/cljc files in the clj repl created with cider-jack-in; it finds cljs files, too, and without any error or repl effect.
  • simultaneously loads clj/cljc files in the clj repl and cljc/cljs files in the cljs repl (from cider-jack-in-clj&cljs, respectively).

Two very minor and harmless effects:

  • When both a clj and a cljs repl are available, the cljs ns names will autocomplete in the clj repl, but of course without any definitions within those cljs-only namespaces. Just the names autocomplete. The cljs ns names do not autocomplete in the clj-only repl case.
  • The *Messages* buffer lists "Loading path/to/file.cljs..." even when there's only a clj repl.

Ready.

@bbatsov bbatsov merged commit 9409acf into clojure-emacs:master Jan 7, 2019
@bbatsov
Copy link
Member

bbatsov commented Jan 7, 2019

Thanks for working on this! I really appreciate it! 🙇

Two very minor and harmless effects:

Probably we should take some time to address those, but I agree that's not a big deal.

@rduplain
Copy link
Contributor Author

rduplain commented Jan 7, 2019

Thanks, and I really appreciate that you maintain many great Clojure tools!

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.

2 participants