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 issues with cider-auto-mode #3355

Merged
merged 5 commits into from
Jun 17, 2023
Merged

Conversation

yuhan0
Copy link
Contributor

@yuhan0 yuhan0 commented Jun 16, 2023

The first two commits fix the cider-mode disabling behavior and performance issues mentioned in #3346.
The latter ones introduce a technically breaking change by changing the default setting of cider-auto-mode, and rewording the getting started section of the user manual to something hopefully less misleading and more beginner-friendly.


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 (eldev test)
  • All code passes the linter (eldev 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)

@yuhan0
Copy link
Contributor Author

yuhan0 commented Jun 16, 2023

Sorry for the noisy commits, I'm not sure why these warnings only show up in the CI and not when I run eldev lint / test locally.

@bbatsov
Copy link
Member

bbatsov commented Jun 16, 2023

@yuhan0 Let's pull the change to the default to a separate PR as that's a breaking change likely to affect a lot of users and I want to discuss the implications better.

Generally, I'm worried that a lot of people will be surprised that cider-mode doesn't get enabled automatically. I've gotten a lot of flak in the past for such changes, so I try to be extra careful with them. :-)

@yuhan0
Copy link
Contributor Author

yuhan0 commented Jun 16, 2023

Ok, I've rebased the non-breaking commits and added on the extra changes from #3354 (going with the double prompt to kill REPLs after choosing not to reuse them)

@yuhan0
Copy link
Contributor Author

yuhan0 commented Jun 16, 2023

Updated the PR description so #3346 doesn't get auto-closed, discussion can be continued there :)

@bbatsov bbatsov merged commit 5d91ffc into clojure-emacs:master Jun 17, 2023
@bbatsov
Copy link
Member

bbatsov commented Jun 17, 2023

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.

2 participants