tweag / topiary

https://topiary.tweag.io/
MIT License
575 stars 29 forks source link

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

Closed nbacquey closed 2 weeks ago

nbacquey commented 2 weeks ago

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:

aspiwack commented 2 weeks ago

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 commented 2 weeks ago

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.

aspiwack commented 2 weeks ago

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.