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

Allow path for grammar source #747

Merged
merged 5 commits into from
Nov 5, 2024
Merged

Conversation

ErinvanderVeen
Copy link
Collaborator

@ErinvanderVeen ErinvanderVeen commented Sep 19, 2024

Allow specifying grammars through prebuild parsers

Description

Previously, Topiary was forced to fetch the grammar repositories and build the grammars manually. This PR adds the option to specify a path to a grammar binary file.

Additionally, it adds the topiary-cli-nix build output, that doesn't fetch any grammars manually.

Checklist

Checklist before merging:

  • CHANGELOG.md updated
  • README.md up-to-date

@ErinvanderVeen ErinvanderVeen force-pushed the allow-path-for-grammar-source branch 2 times, most recently from b95715c to 6ecc054 Compare September 20, 2024 09:47
Copy link
Member

@Xophmeister Xophmeister left a comment

Choose a reason for hiding this comment

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

I've made a few comments about the README, but otherwise it looks good 👍

Although, I don't really follow the Nix-stuff, so you may want a second opinion.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@yannham
Copy link
Member

yannham commented Oct 1, 2024

FYI, I'm trying that as part of a flake.lock update on the Nickel side. Will report if there is any issue with the new -nix only entry.

@yannham
Copy link
Member

yannham commented Oct 1, 2024

One major issues is that the grammar from nixpkgs aren't necessarily up-to-date, so the queries don't match. This might not be the case for more stable grammars, but for example this is the case for the Nickel grammar, to topiary-cli-nix binary isn't able to format Nickel currently. A dirty fix is to take the tree-sitter-nickel grammar as a separate argument that we can pull from the nickel-lang/tree-sitter-nickel flake directly, in the meantime, because I tried to update the grammar upstream directly in Nixpkgs but the process is really not obvious.

@yannham
Copy link
Member

yannham commented Oct 1, 2024

I've pushed my work-around to use the right Nickel grammar. I don't know if this needs to be done for other grammars as well,I haven't tried them all.

@Xophmeister
Copy link
Member

@yannham Your changes look good to me insofar as I can judge, but I trust you so feel free to consider this approved if it's critical for Nickel.

@yannham
Copy link
Member

yannham commented Oct 2, 2024

In the end I could get rid of the Nixified version of Topiary from the Nickel build altogether, so this isn't as critical as before, if you prefer to wait for another look from @ErinvanderVeen or someone else.

@nbacquey nbacquey force-pushed the allow-path-for-grammar-source branch 2 times, most recently from 9c53f07 to 9c8bd40 Compare November 5, 2024 09:50
Erin van der Veen and others added 5 commits November 5, 2024 14:11
This build output uses the grammars from nixpkgs over those provided by
topiary itself.
The tree-sitter Nickel grammar taken from Nixpkgs is too old and doesn't
match current queries, and the update process to upstream an update is
not obvious. In the meantime, we take the tree-sitter-nickel grammar
directly from the source repo - as a flake input - to ensure it's up to
date.
@nbacquey nbacquey force-pushed the allow-path-for-grammar-source branch from 9c8bd40 to cc1f8fc Compare November 5, 2024 13:12
@nbacquey
Copy link
Member

nbacquey commented Nov 5, 2024

I'm not proficient enough in Nix to certify this part of the changes.
But in the meantime, the ability to specify a compiled grammar is useful in itself. I would advocate to merge now, and possibly revise the Nix stuff later, should the need arise.

@ErinvanderVeen
Copy link
Collaborator Author

I'd like to keep the Nixified version as well for now. It doesn't reduce the functionality or behaviour of any other part of Topiary.

@ErinvanderVeen ErinvanderVeen merged commit fc9eafd into main Nov 5, 2024
9 checks passed
@ErinvanderVeen ErinvanderVeen deleted the allow-path-for-grammar-source branch November 5, 2024 15:33
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