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

[#1990] Adds option to auto-save buffers when calling cider-refresh #1994

Merged
merged 1 commit into from
May 25, 2017
Merged

Conversation

rymndhng
Copy link
Contributor

@rymndhng rymndhng commented May 20, 2017

Fixes #1990. Adds an option to auto-save buffers when calling cider-refresh. There is an option cider-prompt-save-file-on-cider-refresh which allows users to customize this behaviour. By default, the user is prompted for each clojure buffer to save.

  • The commits are consistent with our [contribution guidelines][1]
  • All tests are passing (make test)
  • The new code is not generating bytecode or M-x checkdoc warnings
  • You've updated the changelog (if adding/changing user-visible functionality)
  • You've updated the readme (if adding/changing user-visible functionality)

@dpsutton
Copy link
Contributor

The functionality probably doesn't need to be in the body of that let, and it would make sense if the first few lines were

(cider-ensure-connected)
(cider-ensure-op-supported "refresh")
(cider--offer-save-project-files)
(let ((....

This would look nicer as a function so we could sprinkle this around in other locations as well that I'm sure will make sense.

One area of concern: in an IDE we would easily and care-free save all files. In emacs, it's possible we span multiple projects. Should we worry about filtering down to just the current project (using perhaps nrepl-project-dir or (clojure-project-dir)) so that we do not save buffers that are not involved in the refresh operation or is that an overblown concern?

@@ -1541,6 +1552,9 @@ refresh functions (defined in `cider-refresh-before-fn' and
(let ((clear? (member mode '(clear 16)))
(refresh-all? (member mode '(refresh-all 4)))
(inhibit-refresh-fns (member mode '(inhibit-fns -1))))
(when cider-prompt-save-file-on-cider-refresh
Copy link
Member

Choose a reason for hiding this comment

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

That should limited only to project-specific buffers. It would be odd to save all Clojure buffers on refresh IMO.

Copy link
Member

Choose a reason for hiding this comment

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

Not sure if we had some function returning those, implementing something like this should trivial.

@bbatsov
Copy link
Member

bbatsov commented May 20, 2017

@dpsutton is completely right.

@dpsutton
Copy link
Contributor

I haven't felt the pain of this bug as I run prelude and have the autosaving all buffers on by default when switching. Perhaps this is an avenue that we could pursue? Rather than ensuring a particular project's files are saved, allow the user to enable CIDER to always save all buffers. This issue would seemingly go away, and we don't need to worry about project boundaries since the user is the one that gives explicit approval for CIDER to save all buffers all the time?

The functionality is done in prelude through the use of advice. It works well but i've heard function advice is the unwelcome part of emacs lisp, but here's the functionality.

I'm not saying that this is necessarily the correct way forward but it does skirt dealing with comparing file locations and notions of projects while making refreshing easier.

@bbatsov
Copy link
Member

bbatsov commented May 21, 2017

I'm not saying that this is necessarily the correct way forward but it does skirt dealing with comparing file locations and notions of projects while making refreshing easier.

I'm not sure what's the Prelude functionality you're referring to. I know I'll be extremely annoyed if I get warning about the unsaved source buffers in unrelated projects. I also know that getting this right is trivial, so I don't see the point in not doing it properly.

@dpsutton
Copy link
Contributor

dpsutton commented May 22, 2017

the functionality i was talking about was prelude autosaving files when they lose focus:

(advise-commands "auto-save"
                 (switch-to-buffer other-window windmove-up windmove-down windmove-left windmove-right)
                 before
                 (prelude-auto-save-command))

I was just offering a different approach that would get us the same answers. I wouldn't have it ask but just continually save all clj(s|c) as you leave buffers.

I also know that getting this right is trivial, so I don't see the point in not doing it properly.

True. I only wanted to point out a possible alternative.

@bbatsov
Copy link
Member

bbatsov commented May 22, 2017

I was just offering a different approach that would get us the same answers. I wouldn't have it ask but just continually save all clj(s|c) as you leave buffers.

I wrote this a long time ago, but I recall it actually saved just the buffer you were into. It's pointless to save all buffers when you lose focus.

@bbatsov bbatsov merged commit 676a808 into clojure-emacs:master May 25, 2017
@bbatsov
Copy link
Member

bbatsov commented May 25, 2017

I'll merge this and improve it myself.

@rymndhng
Copy link
Contributor Author

@bbatsov thanks for taking it on. Been a bit busy and haven't had the time to follow up on improving my elisp skills!

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