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

Update clojure-ts-mode support #27

Merged
merged 3 commits into from
Feb 18, 2024

Conversation

p4v4n
Copy link
Contributor

@p4v4n p4v4n commented Feb 18, 2024

This is a follow up PR to #26.

@borkdude
Copy link
Owner

Thanks @p4v4n! CI is failing.

cc @kommen

@bbatsov
Copy link
Contributor

bbatsov commented Feb 18, 2024

I see why the CI is failing, but it seems to me it might be better to just depend on a explicit version of seq.el instead of relying on whatever's available in Emacs. The way I see it either we have to downgrade to 2.20 in flycheck or set some explicit dep here as well, so it can be fetched from GNU ELPA if needed.

@bbatsov
Copy link
Contributor

bbatsov commented Feb 18, 2024

Also - might be good to look into abstracting away the test code duplication for the different major modes.

@p4v4n
Copy link
Contributor Author

p4v4n commented Feb 18, 2024

There are 2 issues here.

  1. seq.el issue
    (Raised a ticket upstream as it will also likely affect other flycheck packages)
    and also
  2. I forgot that clojure-ts-mode requires emacs29
    so I will need make a few more changes after seq.el fix.

@p4v4n
Copy link
Contributor Author

p4v4n commented Feb 18, 2024

The downside to using a explicit version of seq.el is that every downstream package(depending on flycheck) needs to keep track of what version of seq flycheck is depending on and update regularly whenever flycheck updates the version upstream.

@kommen
Copy link
Contributor

kommen commented Feb 18, 2024

I forgot that clojure-ts-mode requires emacs29

And also need to install (from source?) the clojure treesitter gramma, right? I think it is already more convenient but haven't tried/looked into how this works now.

@p4v4n
Copy link
Contributor Author

p4v4n commented Feb 18, 2024

And also need to install (from source?) the clojure treesitter gramma, right? I think it is already more convenient but haven't tried/looked into how this works now.

Luckily its already handled by clojure-ts-mode automatically with clojure-ts-ensure-grammars set to true by default.
The tests are already passing on the emacs 29+ for this branch.
https://github.com/borkdude/flycheck-clj-kondo/actions/runs/7945808126/job/21697659833

I think we only need to make sure that the clojure-ts-mode tests are not run when emacs-version < 29.1.

@borkdude borkdude merged commit e38c67b into borkdude:master Feb 18, 2024
7 checks passed
@borkdude
Copy link
Owner

Thanks!

@p4v4n
Copy link
Contributor Author

p4v4n commented Feb 18, 2024

Thanks @borkdude!

I was about to make a few more changes to this PR (like moving ts tests to a new file and other minor stuff). Will raise a separate PR later as it is not critical.

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.

4 participants