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

Optionally specify callback to cider load buffer #2689

Conversation

magnars
Copy link
Contributor

@magnars magnars commented Aug 10, 2019

  • cider-load-buffer (normally bound to C-c C-k) does not pass a callback to cider-request:load-file. With this change, a user can now optionally specify this callback.

  • when no callback is passed to cider-request:load-file, it will fall back on cider-load-file-handler, which does not specify a done-handler. With this change, a user can now optionally specify a done-handler.

These two changes together would allow users to perform some task after loading the current buffer, by doing something like this:

(cider-load-buffer nil (cider-load-file-handler nil (lambda (&rest _) (message "I'm done!"))))

In my case that would be running tests.


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
  • [-] 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 Aug 10, 2019

Nice idea! Let me think a bit about this, as there's another approach to do the same thing - namely trigger some hook functions in the done handler, which might be more user-friendly.

@bbatsov
Copy link
Member

bbatsov commented Aug 10, 2019

Let me also ping @Malabarba here, as back in the day he had a different approach to this https://endlessparentheses.com/test-driven-development-in-cider-and-emacs.html

@bbatsov
Copy link
Member

bbatsov commented Aug 10, 2019

Hmm, actually I see we already have the file loaded hook, but it's being triggered in another handler (the value handler). I'm not sure if this has any practical differences, so I guess you can help me understand why using the done callback is preferable for this.

@magnars
Copy link
Contributor Author

magnars commented Aug 10, 2019

I'm happy to use the cider-file-loaded-hook for my use case. I just didn't notice it there, since I was thinking in callbacks. :) I still see some use cases for this change, in that hooks are global. When passing in the callback you can affect only this invocation, without having to juggle un-registering hooks afterwards.

@magnars
Copy link
Contributor Author

magnars commented Aug 10, 2019

As a side note, the trick @Malabarba uses is the one I'm trying to avoid here - since I had to resort to a (reset) in my save-handler, which doesn't scale well with project size.

@bbatsov
Copy link
Member

bbatsov commented Aug 11, 2019

Btw, I've noticed one small problem with our existing hook - starting with nREPL 0.6 you can actually get multiple value responses (chunks of the same result). While this is unlikely to affect ns loading, it might also be better to move the triggering of the hook to the done handler as well.

When passing in the callback you can affect only this invocation, without having to juggle un-registering hooks afterwards.

Yeah, that's a perfectly valid argument.

@@ -501,7 +502,7 @@ or it can be a list with (START END) of the evaluated region."
(lambda (_buffer err)
(cider-emit-interactive-eval-err-output err)
(cider-handle-compilation-errors err eval-buffer))
'()
(or done-handler '())
Copy link
Member

Choose a reason for hiding this comment

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

since '() is the same as nil, this whole line is the same as just done-handler

@Malabarba
Copy link
Member

I think it's reasonable for the function to receive a callback argument, since it's already implemented in the lower functions. I think both the hook and the local callback will probably have its uses in different situations.

@bbatsov
Copy link
Member

bbatsov commented Aug 15, 2019

@Malabarba Fine by me. Let's get this baby merged! Btw, do you think we should move the hook in the done handler as well?

@magnars Please, add a changelog entry for the PR.

@magnars magnars force-pushed the optionally-specify-callback-to-cider-load-buffer branch from 382c2b0 to 1c10aff Compare August 15, 2019 12:56
@magnars
Copy link
Contributor Author

magnars commented Aug 15, 2019

Done, as well as freshly rebased onto master.

@bbatsov bbatsov merged commit 87f750b into clojure-emacs:master Aug 20, 2019
@bbatsov
Copy link
Member

bbatsov commented Aug 20, 2019

Thanks!

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.

3 participants