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

No longer merge config files by default, use priorities instead #790

Merged
merged 4 commits into from
Nov 7, 2024

Conversation

nbacquey
Copy link
Member

@nbacquey nbacquey commented Nov 6, 2024

Description

At the moment, the way Topiary configuration works is counterintuitive: the README suggests that configuration files are somehow prioritized, and that a higher-priority file trumps a lower-priority one.

This is not the case. Because of how Nickel merge works, all configuration files are actually equivalent, which has led to problems when a configuration passed with the -C option disagrees with, for instance, the built-in configuration. Although this behavior is documented in the README, I think it's highly counterintuitive.

This PR switches the default behavior to priority-based, meaning that all lower-priority files will be disregarded.
The newly added flag --merge-configuration reinstates the old behavior.

Closes #612 (sort of)

Checklist

Checklist before merging:

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

@nbacquey nbacquey changed the title Nb/config rework No longer merge config files by default, use priorities instead Nov 6, 2024
@nbacquey nbacquey marked this pull request as ready for review November 6, 2024 15:17
@nbacquey nbacquey requested a review from Xophmeister November 6, 2024 15:17
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.

🙏

@aspiwack
Copy link
Member

aspiwack commented Nov 7, 2024

I wonder if either is the right behaviour, though. Do you actually want options that you haven't specified on the local configuration file to override the ones from the global configuration file? This doesn't seem like the general case.

Your feeling is that you shouldn't have to write | force in the local configuration to override an option (if I'm reading you correctly). You're saying that there's a natural hierarchy between files and that you really want there to be a priority. But I assume the intuitive behaviour is per-option priority. Or do I get it wrong?

Could we document what ideal semantics we'd like to have (regardless of whether it's feasible with Nickel)?

@nbacquey
Copy link
Member Author

nbacquey commented Nov 7, 2024

I'm not sure there is a single right behavior, to be honest.

I'm not sure either I understand what you mean by "per-option priority", could you please elaborate on that? It sort of makes sense to think about per-language priority, but at the moment a "language" isn't well defined in the configuration file.

You're correct regarding one of the goal of this PR: I don't want to have to specify | priority 1 every time I want to override something from the built-in configuration. However, there is another benefit to the behavior introduced in this PR: for the sake of reproducibility, we want to be able to completely specify the configuration of Topiary at runtime, and make it independent from, say, system-wide or built-in configuration files. This was the original point of #612, here.

In the end, I'd argue that even if I don't know what the ideal semantics of multiple configuration files should be, the behavior introduced in this PR is something we want to have no matter what.

What's open to discussion is whether it should be the default behavior.
I think that it should, just because it's simpler to understand.

@nbacquey nbacquey merged commit ba8a2e0 into main Nov 7, 2024
9 checks passed
@nbacquey nbacquey deleted the nb/config-rework branch November 7, 2024 16:36
@aspiwack
Copy link
Member

aspiwack commented Nov 8, 2024

I'm not sure either I understand what you mean by "per-option priority", could you please elaborate on that? It sort of makes sense to think about per-language priority, but at the moment a "language" isn't well defined in the configuration file.

It's not necessarily something very precise. But maybe you have foo.bar's value sourced from the global configuration, while foo.baz is defined locally, and so overrides the global configuration's definition.

You're correct regarding one of the goal of this PR: I don't want to have to specify | priority 1 every time I want to override something from the built-in configuration.

Maybe a better way would be to have the gobal configuration file to be only default. But it has its limitation: maybe you want a configuration in /etc and one in $XDG_CONFIG, and then one local. There's no good way to annotate priorities on the middle one so that 1/ they take precedence on the /etc configuration file 2/ you don't have to write explicit priorities in the local configuration file.

However, there is another benefit to the behavior introduced in this PR: for the sake of reproducibility, we want to be able to completely specify the configuration of Topiary at runtime, and make it independent from, say, system-wide or built-in configuration files

I agree, it's akin to the --pure mode of nix-shell. But I'm questioning whether it should be the default. Maybe it should, but it's not intuitive to me.

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.

Allow the setting of the collation mode in the configuration file
3 participants