-
Notifications
You must be signed in to change notification settings - Fork 47
fix(config): Set built-in language configs with a priority of "default" #1126
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
fix(config): Set built-in language configs with a priority of "default" #1126
Conversation
|
@tweag/topiary-core-team we'll need to cut a release for this fix so that |
|
Is there anything that's needed to make this PR land on |
|
Hi @Niols I was on vacation so the release was a bit delayed. I should have it out for review tomorrow. |
|
Oh. Sorry, I didn't want to put pressure on your vacation or anything! I was just curious why the PR wasn't on |
|
This has the effect of making all of |
@yannham I'm assuming the built-in values should have the lowest merge priority individually, to your point though we should have every language field have its own Nice catch! |
|
@yannham Thanks again for the per language |
This reverts commit 2c57d70.
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.
Looks good; thanks @mkatychev 🙏
I've set the release date, in the CHANGELOG for tomorrow. I can sort that out 👍
|
I now get $ topiary format -M -C .topiary.ncl lib/name.ml
[2025-11-25T17:31:57Z ERROR topiary::error] Nickel error: Other(
"missing field `extensions`",
)when trying my usual configuration {
languages = {
bash.indent | priority 1 = " ",
css.indent | priority 1 = " ",
json.indent | priority 1 = " ",
nickel.indent | priority 1 = " ",
ocaml.indent | priority 1 = " ",
ocaml_interface.indent | priority 1 = " ",
ocamllex.indent | priority 1 = " ",
rust.indent | priority 1 = " ",
toml.indent | priority 1 = " ",
},
} |
|
Reverting nixpkgs to |
|
Hmm oh boy, looks like the fix broker the other things. @toastal the config above reproduces the issue I take it? |
|
@mkatychev … yeah, my Nickel is a bit rusty at this point, but maybe something isn’t squaring right for me |
|
Thanks for bringing this up, I'll look into it first thing tomorrow |
Set built-in language configs with a priority of "default"
Resolves #1124
Description
New built-in config behaviour caused merge priority collisions with existing configs.
This PR sets the merge priority of the built-in configuration to
defaultfor all languagesChecklist
Checklist before merging, wherever relevant:
CHANGELOG.mdupdatedREADME.md, etc.) up-to-date