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 language definition #3387

Merged
merged 3 commits into from
Aug 10, 2022
Merged

Conversation

IGJoshua
Copy link
Contributor

Hello,

I'd like some feedback on this PR since I don't fully understand all the fields that are being set in this table.

Does this edit look complete, or does the injection regex need to be updated in order to match some of the other file types, like edn and boot?

Otherwise, this is a simple update to add support for Clojure files supported by clojure-lsp but which aren't reflected in helix's language definition, and to correct the comment token.

File types

  • cljs for ClojureScript
  • cljc for cross-compiled Clojure files
  • clje for Clojerl
  • cljr for ClojureCLR
  • cljx for legacy cross-compiled Clojure
  • edn for Extensible Data Notation files, they are to Clojure what JSON is to JS
  • boot for build files using the boot build tool

The last thing I'd like confirmation on is that roots defines a set of alternate files to detect a project root, and not that all must be present to detect a project, as the files here are for the major build tools, and it's quite common to have only one of these and none of the others.

languages.toml Outdated
Comment on lines 1529 to 1530
file-types = ["clj" "cljs" "cljc" "clje" "cljr" "cljx" "edn" "boot"]
roots = ["project.clj" "build.boot" "deps.edn" "shadow-cljs.edn"]
Copy link
Member

Choose a reason for hiding this comment

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

for TOML arrays you'll need commas between the elements

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, thanks for the catch! It's been a while since I manually edited a toml file.

@the-mikedavis
Copy link
Member

This will update the auto-generated docs as well so you'll need to run cargo xtask docgen and commit the results

@the-mikedavis
Copy link
Member

I think changing the regex to include boot and edn makes sense 👍

The last thing I'd like confirmation on is that roots defines a set of alternate files to detect a project root, and not that all must be present to detect a project

Yep, this is the case. The relevant code is here: we find the workspace root (so we can tell the language server about it) by checking for any of the root markers or a .git directory

@IGJoshua
Copy link
Contributor Author

I've fixed the missing commas, updated the regex, and I ran cargo xtask docgen but it produced no diffs, so no commit was added for that.

@the-mikedavis
Copy link
Member

Ah yeah sorry I read the diff wrong 😅, there shouldn't be any changes

@the-mikedavis the-mikedavis merged commit d192d59 into helix-editor:master Aug 10, 2022
@the-mikedavis
Copy link
Member

Thanks!

AlexanderBrevig pushed a commit to AlexanderBrevig/helix that referenced this pull request Aug 29, 2022
thomasskk pushed a commit to thomasskk/helix that referenced this pull request Sep 9, 2022
@sogaiu
Copy link
Contributor

sogaiu commented Jan 29, 2023

@IGJoshua and any others concerned, as far as I'm aware, ClojureCLR uses clj just like the JVM Clojure.

See the following for some evidence:

If you know otherwise, please let me know as that might be relevant for tree-sitter-clojure.

@IGJoshua
Copy link
Contributor Author

For clarity, the ClojureCLR port of Clojure can and will use .clj filetypes, however it will first look for .cljr filetypes to allow a single library to be created with host-specific namespaces. This is a part of the support for the #?(:clj ... :cljr ... :cljs ...) syntax for reader conditionals in .cljc files.

https://github.com/clojure/clojure-clr/blob/master/Clojure/Clojure/Lib/RT.cs#L3229

@sogaiu
Copy link
Contributor

sogaiu commented Jan 31, 2023

@IGJoshua Thanks a lot for the explanation!

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