-
Notifications
You must be signed in to change notification settings - Fork 90
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
Topiary integration as nickel format
#1371
Conversation
63297d4
to
5f8fefc
Compare
5f8fefc
to
e8da188
Compare
Improve error reporting for `nickel format` nix build with topiary Use upstream topiary query export
e8da188
to
bc131dc
Compare
I'm marking this as ready for review, even though static linking isn't quit figured out yet. The formatting subcommand is gated behind a feature flag and currently disabled for the static build. We can enable the feature flag for the static build once we figure out how to get it to work. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The structure of the CLI looks better like this 👍 I'm requesting changes to not forget to disable the format feature for now, until we figure out the static build story.
flake.nix
Outdated
nickel-lang-cli = buildPackage { pname = "nickel-lang-cli"; }; | ||
nickel-lang-cli = buildPackage { | ||
pname = "nickel-lang-cli"; | ||
extraBuildArgs = "--features format"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't we want to disable it by default for this PR, until we figure out the static binary build story?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can keep it enabled for the normal CLI build, and just disable it for the static binary. That way at least nix run github:tweag/nickel
gets the formatting feature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, what I understood as "principle of least surprise" from the last planning meeting was that the binary built/run from nix with the default command and the one provided as a static binary shouldn't differ, as you should expect as a user. I would vote for disabling everywhere for now, but just merge it, so that this PR doesn't drift away from the master branch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's what I meant. But I don't think you need to choose how you're going to handle it before you release.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Alright, I've disabled the format
feature for the nix build 👍
Add a
nickel format
subcommand to the CLI, gated behind theformat
feature flag. The nix flake outputsnickel
andnickel-static
enableformat
by default.The
nickel format
subcommand is a thin wrapper around Topiary and embeds the required tree-sitter queries for Nickel formatting.Status: