Add preliminary support for clojure-ts-mode

[dannyfreeman]

This does NOT intended to break the dependency from cider to clojure-mode. It is intended to make CIDER work with clojure-ts-mode.

Some functionality like clojure-find-ns, clojure-find-def, etc will still require clojure-mode in order for CIDER to get it's work done.

I'm at a loss for how to run the tests for this project locally. If there are any tests I could write that make sense for these changes I am open to suggestions.

PR is in draft mode because I still need to see where it makes sense to include references to clojure-ts-mode in the documentation.

---

• 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
  • byte-compilation, checkdoc, check-declare, packaging metadata, indentation, and trailing whitespace checks.
• 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.

[vemv]

I'm at a loss for how to run the tests for this project locally

make test

Will have a look tomorrow!

[dannyfreeman]

I'm at a loss for how to run the tests for this project locally

make test

Yes, I had some trouble because I use nix and there were some missing dependencies. I think the existing tests are working now. Sometimes they fail locally but that's because they time out (I develop on a pinebook most of the time).

Will have a look tomorrow!

Thank you!

[bbatsov]

There's some docs about the tests here https://docs.cider.mx/cider/contributing/hacking.html#testing-the-code Feel free to improve it if something is missing.

[bbatsov]

Some functionality like clojure-find-ns, clojulore-find-def, etc will still require clojure-mode in order for CIDER to get it's work done.

Do those functions work properly in clojure-ts-mode buffer?

[bbatsov]

Overall the changes look fine to me, I'm mostly concerned what happens when you call out to the clojure-mode functions in a buffer with a different mode. I'm guessing they can't work properly without the locals and the syntax table.

[vemv]

It's looking good to me as well!

[dannyfreeman]

Some functionality like clojure-find-ns, clojulore-find-def, etc will still require clojure-mode in order for CIDER to get it's work done.

Do those functions work properly in clojure-ts-mode buffer?

I've tested clojure-find-ns and clojure-find-def , both of which work in clojure-ts-mode buffers. It could be they work because clojure-ts-mode uses the same syntax-table as clojure-mode (See clojure-emacs/clojure-ts-mode#14). Cider functionality like cider-repl-set-ns that I assume is built on top of clojure-find-ns works as well.

There are some other clojure functions to be tested with this. After work I will see about hunting them down and testing them all. This seems like a good candidate for an automated test.

[vemv]

There are some other clojure functions to be tested as well. After work I will see about hunting them down and testing them all. This seems like a good candidate for an automated test.

Perhaps you have played as well with the idea of making clojure-mode's test suite succeed with clojure-ts-mode?

It might be not too hard to hack it for accomplishing so. One can always exclude certain tests (our Eldev shows a test selector setup) if some of them don't make sense for clojure-ts.

[bbatsov]

Perhaps you have played as well with the idea of making clojure-mode's test suite succeed with clojure-ts-mode?

I was just about to ask @dannyfreeman about this on the clojure-ts-mode's issue tracker. Now that semantic indentation has been implemented he can just copy the test suite and disable whatever has not been implemented yet.

[bbatsov]

I've tested clojure-find-ns and clojure-find-def , both of which work in clojure-ts-mode buffers. It could be they work because clojure-ts-mode uses the same syntax-table as clojure-mode (See clojure-emacs/clojure-ts-mode#14). Cider functionality like cider-repl-set-ns that I assume is built on top of clojure-find-ns works as well.

If the syntax-table is the same, they should work.

[dannyfreeman]

I am truly at a loss for how I can please the byte compiler with this error on Emacs 28

https://app.circleci.com/pipelines/github/clojure-emacs/cider/1865/workflows/5ce3f27a-e197-43c9-9638-0625ed6b3c8d/jobs/9920/steps

I have tried wrapping this code https://github.com/clojure-emacs/cider/pull/3461/files#diff-c53d920d1936b521743bbcd39f4f3c479f2122d4d051c30501310f6d6afc01bfR2112-R2115
with error suppression macros like with-suppressed-warnings and the more coarse grained with-no-warnings. I cannot install clojure-ts-mode explicitly on Emacs 28 because of it's requirement on Emacs 29. I am unsure how to proceed.

[dannyfreeman]

I am truly at a loss for how I can please the byte compiler with this error on Emacs 28
...
I am unsure how to proceed.

All it takes is one desperate PR comment to find something that actually works they way you want it to. 1858ab4

Is something like this fix acceptable? Supressing free-var warnings specifically for clojure-ts-mode-map?

Now that this is in place I'll see about further automated tests.

[bbatsov]

@dannyfreeman What's your timeline for this PR? Do you want to get it in time for CIDER 1.8 or for the next release? I'm asking mostly because I hope that we'll finally release CIDER 1.8 in the next few days. (I know I've said this a few times already :D )

[dannyfreeman]

@dannyfreeman What's your timeline for this PR? Do you want to get it in time for CIDER 1.8 or for the next release? I'm asking mostly because I hope that we'll finally release CIDER 1.8 in the next few days. (I know I've said this a few times already :D )

I think the only thing left is to get tests running against clojure-ts-mode. I can try to get them working Saturday, but won't have much time before then. It would be nice to get it in 1.8, but I don't want to hold up the release either.

[dannyfreeman]

I guess I do have some outstanding things to figure out with the tests to run with clojure-ts-mode. Just to name a few

What tests make sense to run? Some do not (extra font locking for example, since that is not implemented).

When we run the test in CI/CD we will need to make sure that the tree-sitter-clojure-grammar gets installed so the modes don't error out on startup. Emacs has facilities for automating this, but it also requires a C compiler and an internet connection. That may prove to be troublesome.

What is the best way to make sure clojure-ts-mode installs only on Emacs29 on the test machine? Similarly, how should certain tests be run conditionally?

When clojure-ts-mode is installed it effectively "hijacks" clojure-mode using the new major-mode-remap-alist functionality in Emacs. That could also be troublesome. Maybe the answer is a new test suite that install clojure-ts-mode, the grammar, and runs everything

[vemv]

Emacs has facilities for automating this, but it also requires a C compiler and an internet connection.

You are free to add extra steps to https://github.com/clojure-emacs/cider/blob/master/.github/workflows/test.yml which similarly, downloads and installs misc stuff.

(Note that adding it to GH Actions is enough; CircleCI will likely be dropped due to its erratic performance)

What is the best way to make sure clojure-ts-mode installs only on Emacs29 on the test machine?

Probably our Eldev file can host such logic.

Similarly, how should certain tests be run conditionally?

By creating a new Eldev group. We have e.g. integration. In this PR I added a group named enrich (note the Eldev part of the diff): https://github.com/clojure-emacs/cider/pull/3472/files

One can also have conditions at the GH Actions level, see if: "!startsWith(matrix.os, 'windows')" in the same PR.

When clojure-ts-mode is installed it effectively "hijacks" clojure-mode using the new major-mode-remap-alist functionality in Emacs. That could also be troublesome.

I didn't get this point. Would it be troublesome for the existing tests, or for end users?

Cheers - V

[dannyfreeman]

When clojure-ts-mode is installed it effectively "hijacks" clojure-mode using the new major-mode-remap-alist functionality in Emacs. That could also be troublesome.

I didn't get this point. Would it be troublesome for the existing tests, or for end users?

For tests. This setting is fine for users (as far as I know, no one has complained).

If clojure-ts-mode has been installed, any call to clojure-mode will instead call clojure-ts-mode (and similarly for derivative modes). So if clojure-ts-mode was installed for regular test runs, all the tests would start running against clojure-ts-mode instead of clojure-mode like they do now. Because of that I think we will want an isolated test run just for clojure-ts-mode. From the rest of your comments it seems like this will be do-able.

[dannyfreeman]

I did a rebase to get rid of what github thought was a merge conflict (there wasn't one?). That was needed to make the github CI run.

[vemv]

Nice commit!

So if clojure-ts-mode was installed for regular test runs, all the tests would start running against clojure-ts-mode instead of clojure-mode like they do now.

This has an upside. Consider, for example the describe "cider-defun-at-point" test. Ideally we would run it once for clojure-mode, and once for clojure-ts-mode.

This way we ensure that CIDER stuff works no matter the clojure.*mode.

[dannyfreeman]

So if clojure-ts-mode was installed for regular test runs, all the tests would start running against clojure-ts-mode instead of clojure-mode like they do now.

This has an upside. Consider, for example the describe "cider-defun-at-point" test. Ideally we would run it once for clojure-mode, and once for clojure-ts-mode.

This way we ensure that CIDER stuff works no matter the clojure.*mode.

I've been studying how the redirection works. If you manually invoke clojure-mode, Emacs does not do any kind of major-mode redirect. That makes sense. A user might want to explicitly call clojure-mode. All the major-mode-remap-alist does is remap major mode calls that are invoked from things like auto-mode-alist. So, all tests that invoke clojure-mode still just invoke clojure-mode. So we don't get free tests against clojure-ts-mode everywhere in that regard. It will take a little more work than that

[bbatsov]

I think the only thing left is to get tests running against clojure-ts-mode. I can try to get them working Saturday, but won't have much time before then. It would be nice to get it in 1.8, but I don't want to hold up the release either.

It seems we won't cut CIDER 1.8 before the end of this week, so you still have time. I'm hoping to finally ship something by the middle of next week.

[bbatsov]

Before we merge this PR you can squash this to fewer commits - e.g. one for the changes needed in CIDER and a second one for the CI updates. (or something along those lines)

[dannyfreeman]

It seems we won't cut CIDER 1.8 before the end of this week, so you still have time. I'm hoping to finally ship something by the middle of next week.

Understood, I always appreciate some extra time. I'll try not to push it though :)

Before we merge this PR you can squash this to fewer commits - e.g. one for the changes needed in CIDER and a second one for the CI updates. (or something along those lines)

Yes I planned on doing that eventually. I'll probably need some more iterations running against the CI server before then.

[ikappaki]

Tagging here @ikappaki in case he's willing to help with the Windows tests.

Hi all, sorry for the late reply.

Both the aforementioned tests are expected to fail when run on GitHub CI, even without these changes, due to incidental environment issues. They run fine on circleci.

If you happen to remember from some time ago, I've made a suggestion to migrate the circleci jobs to GHA with #3207, though we've decided we are fine with the current setup.

Although I did not mention it at the time, I've noticed and corrected these two failures in one of my unpublished branches at master...ikappaki:cider:tech/github-ci-all .

The first issue with cider-expected-ns that @dannyfreeman hacked, has to do with the root directory d:\a existing on the GH windows machine by default, and when we ask in the test it to resolve /a in the test it resolves it to d:\a, which is correct. The fix did in the above branch is to use different names /cider--a instead etc

The second issue, eval, is caused by a hardcoded /tmp directory (if I remember well by looking at the patch), which is not available on the github machine (fix is to use tmp-dir instead).

Are we planning to run the clojure-ts-mode tests on GH though? They are akin to the unit test suit already running on circleci and I think they should be bundled with them.

Nevertheless, I can open a PR with just the fixes for these two tests.

Thanks

[vemv]

Thanks much for the accurate analysis @ikappaki !

I think it would be fine to follow the current strategy (i.e. GH being a superset of Circle because it includes integration tests).

From my side I don't like CCI as much as I once did, but they stilll have some value (especially after #3500 - now only CCI builds on every commit, which is useful for fast feedback).

Looking forward to the PR.

[ikappaki]

Looking forward to the PR.

Fixed the two GHA issues with #3532.

[dannyfreeman]

Thank you @ikappaki for addressing the windows bugs. I think I have successfully removed the workarounds I put in place and have all my changes rebased on top of master branch.

Apologies for the slow response here. I had some things with my family come up that required some traveling, been busy with all that. I'm getting back in the groove of working on this now.

Edit:
Luck is with me tonight. Tests passed on windows with the clojure-ts-mode run. I'll be shifting my next bit of work over to clojure-ts-mode and getting tests over there working.

[yenda]

@dannyfreeman what's left to do? this looks awesome thanks for doing this great work!

[dannyfreeman]

what's left to do?

The test suite from clojure-mode needs to be ported to clojure-ts-mode. I plan to use some upcoming vacation time to tackle this, as I think it's going to be a decent amount of work.

[purcell]

Looking forward to this PR landing! (Not a useful comment, certainly, but an opportunity to thank @dannyfreeman for clojure-ts-mode and this work, plus those giving feedback on the PR.)

[bitti]

font-lock got broken for me in clojure-mode a while ago, but I didn't have the nerve to debug it, so I switched to clojure-ts-mode since then. Currently Cider is working fine for me though, just by adding (add-hook 'clojure-ts-mode-hook #'cider-mode) and patching cider-repl-type-for-buffer to include clojure-ts-mode.

But it's annoying having to do the patch after each update. If this PR still takes a while, it would be nice if just this one change can be done first.

[bbatsov]

@bitti Well, it's perfectly fine for someone else to try to push the necessary changes faster. As I wrote earlier in the discussion here for me the tests were kind of optional at this point and I don't mind cutting a few corners in the interest of making the lives of end users better faster.

I think it's still a good idea to report a bug for whatever went wrong with clojure-mode for you, though. Might be an easy fix.

[bbatsov]

@p4v4n @kommen Will one of you be interested in picking up this PR and getting it to mergeable state? (we can tackle porting the tests to clojure-ts-mode separately)

[kommen]

@bbatsov yep, I'm interested. I might even have some time this weekend to hack on it. @p4v4n if you are interested as well, feel free to hit me up in the Clojurians Slack to coordinate.

[kommen]

@bbatsov I have opened #3622 as a continuation of this PR here, as I couldn't find a way to use this PR. Let me know if you want to use a different approach.

[vemv]

SGTM, please only check that there aren't pending items (other than tests)

If there are, please add them as a checklist item in the PR or maybe create an issue in the https://github.com/clojure-emacs/clojure-ts-mode repo?

[dannyfreeman]

Thanks for picking this up. I didn't have any other pending things in this PR other than tests on the clojure-ts-mode side. I have not had enough free time to be able to work on those.

[bbatsov]

Thanks for picking this up. I didn't have any other pending things in this PR other than tests on the clojure-ts-mode side. I have not had enough free time to be able to work on those.

No worries. Let's just open an issue in the other repo to track this and we'll get it done, when we can. For now it was important to make it easier for people to use CIDER with clojure-ts-mode and I think we made some good progress in this direction.

[dannyfreeman]

No worries. Let's just open an issue in the other repo to track this and we'll get it done, when we can. For now it was important to make it easier for people to use CIDER with clojure-ts-mode and I think we made some good progress in this direction.

Yeah for sure. we already have an issue open over there clojure-emacs/clojure-ts-mode#25