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

Add support for janet language #4617

Closed

Conversation

KittyBorgX
Copy link

Resolves : #4574

@KittyBorgX
Copy link
Author

I'm sorry for the force pushes 😅 I forgot to add a few things and I didn't want to stack commits, so I rebased and pushed again.

@kirawi kirawi added S-waiting-on-review Status: Awaiting review from a maintainer. A-language-support Area: Support for programming/text languages labels Nov 7, 2022
@the-mikedavis
Copy link
Member

Since Janet is a lisp and looks a lot like clojure, I wonder if we can re-use the clojure or scheme parsers and only write new queries?

@KittyBorgX
Copy link
Author

Since Janet is a lisp and looks a lot like clojure, I wonder if we can re-use the clojure or scheme parsers and only write new queries?

Does "new queries" mean injections and highlights in runtime/queries ?
I'm sorry, I'm kind of new to this repo and I'll take some time to absorb and understand stuff 😅

@the-mikedavis
Copy link
Member

Yep correct. To share the same parser you will need to change the languages.toml entry to remove the new [[grammar]] sub-table and add a grammar = "clojure" or grammar = "scheme" entry to the new [[language]] sub-table. Then it will need queries in runtime/queries/janet

@KittyBorgX
Copy link
Author

Gotcha. Although, I do have a concern. Looking further into the syntax of all the 3 languages [ janet, clojure and scheme ], there seems to be quite a bit of difference between the 3 of them with respect to keywords and the general mechanics of the languages. Janet does sort of represent scheme and clojure in a way but I feel that it is different enough to have its own grammar (since reusing grammar may raise questionable results and/or errors in the language later?)

Having said this, I could very well be wrong and I'm happy to correct myself if I am. If you feel that the grammar of clojure or scheme can be resused, then let me know, I'll make the necessary changes and add a new commit.

@KittyBorgX
Copy link
Author

@the-mikedavis [Sorry for the mention!] Any update on this PR?

@the-mikedavis
Copy link
Member

Hmm yeah, we might as well pull in a new grammar for this - when compiled, this parser is only 174KB. We can work on deduplicating grammars later, maybe we'll need to write a new grammar that's general enough to be used for any lisp.

@KittyBorgX
Copy link
Author

Hmm yeah, we might as well pull in a new grammar for this - when compiled, this parser is only 174KB. We can work on deduplicating grammars later, maybe we'll need to write a new grammar that's general enough to be used for any lisp.

Alright! Is there any pending work to be done on my end? (I'm really sorry for going back and forth over this PR) 😅

@the-mikedavis
Copy link
Member

Yep this still needs runtime/queries/janet/highlights.scm queries https://docs.helix-editor.com/master/guides/adding_languages.html#queries

@KittyBorgX
Copy link
Author

@the-mikedavis Is it alright if I reuse https://github.com/helix-editor/helix/blob/master/runtime/queries/clojure/highlights.scm with a bit of modification for janet atm? I know I'd told against it but I'm not finding enough time to roll out a new one for janet.

@the-mikedavis
Copy link
Member

Yep that would be fine as long as the syntax is close enough. You can set the grammar to use the clojure one and then inherit the highlights:

# /languages.toml

[[language]]
name = "janet"
# .. other stuff ..
grammar = "clojure"

then in runtime/queries/janet/highlights.scm:

; inherits: clojure

to use all of the clojure highlight patterns, or just use the patterns that are needed

@KittyBorgX
Copy link
Author

Thanks! Btw, How do i resolve the merge conflict? 😅
Do I just pull the latest upstream changes and commit everything again?

@the-mikedavis
Copy link
Member

the-mikedavis commented Nov 18, 2022

You can pull down the master branch from this repo and git merge against that:

git remote add upstream https://github.com/helix-editor/helix
git fetch upstream
git merge upstream/master

(Or if you meant which side to take in the conflict: you can add both new entries so that you have

[[language]]
name = "janet"
# ...

[[grammar]]
name = "janet"
# ...

[[language]]
name = "bicep"
# ...

[[grammar]]
name = "bicep"

)

@KittyBorgX
Copy link
Author

KittyBorgX commented Feb 7, 2023

(first off, im really sorry for the delay)
@the-mikedavis I tried merging the conflicts but its only gotten worse. I want to complete the changes you suggested in this PR. I get "Your local changes to the following files would be overwritten by merge".
I could squash and rebase to head and redo my changes which would erase my commits and I can just make a new one while still being part of this pr.
Any advice?

@howard36
Copy link
Contributor

howard36 commented Feb 7, 2023

It sounds like you have uncommitted changes in your local branch. You should commit them first before running git merge upstream/master

@the-mikedavis
Copy link
Member

Since there aren't a lot of lines changed, starting from scratch like you said might be the easiest option. You can also try committing the local changes before the merge as @Plasma-Vortex says or stashing: git stash before merging and then git stash pop afterwards.

@the-mikedavis the-mikedavis added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from a maintainer. labels Feb 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-language-support Area: Support for programming/text languages S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add support for Janet
4 participants