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 cider-find-keyword for clojure-ts-mode #3779

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

rrudakov
Copy link
Contributor

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. There are warnings, but not because of my changes.
  • You've updated the changelog (if adding/changing user-visible functionality)
  • You've updated the user manual (if adding/changing user-visible functionality)

Thanks!

Currently cider-find-keyword doesn't work with clojure-ts-mode, because it relies on clojure-keyword-face text property (which is different for keywords for clojure-ts-mode). I've introduced a small helper which uses text property for clojure-mode and tree sitter based predicate for clojure-ts-mode.

@rrudakov rrudakov marked this pull request as draft February 27, 2025 17:05
@rrudakov
Copy link
Contributor Author

Looks like it's not that simple. CIDER currently doesn't have clojure-ts-mode as a dependency.

@rrudakov rrudakov force-pushed the master branch 4 times, most recently from 4c9e217 to 53c9ece Compare February 27, 2025 17:34
@rrudakov
Copy link
Contributor Author

Not sure how to fix the linter error. Any suggestions are welcome.

The treesit-* functions are only used if treesit feature is available, but I don't know how to tell linter about it.

@rrudakov rrudakov force-pushed the master branch 2 times, most recently from 7699e11 to b86246c Compare February 27, 2025 17:45
@rrudakov rrudakov marked this pull request as ready for review February 27, 2025 18:21
@rrudakov
Copy link
Contributor Author

I've figured it out :)

@rrudakov rrudakov force-pushed the master branch 2 times, most recently from 90f0a9b to b6d834b Compare March 3, 2025 15:31
@bbatsov
Copy link
Member

bbatsov commented Mar 4, 2025

Let me think about this a bit, as I've got a small concern about leaking implementation details from the major modes in CIDER and I'm wondering what's the best way to abstract this away. Some work will definitely need to be done in this directly to fully embrace clojure-ts-mode. (unless it takes so much time to do it, that we'll merge the two modes first, as is scheduled for Emacs 32 :D )

@rrudakov
Copy link
Contributor Author

rrudakov commented Mar 4, 2025

Sure, sounds good to me. We could probably define some helper in cider-ts-mode (something like clojure-ts-keyword-at-point-p), but I wasn't sure that it's a good idea, it would be needed only to support CIDER.

@bbatsov
Copy link
Member

bbatsov commented Mar 4, 2025

That's what I was thinking as well, otherwise I would have done it by now. Perhaps we can wrap all direct calls to clojure-mode in some cider-foo-bar functions in the same file in CIDER, so we can all such glue code is contained within a single file. No better ideas come to mind right 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.

2 participants