-
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
[Feature] Merge priorities #829
Changes from all commits
7a758f8
288e6fd
912f6fd
27d1b57
18d0a4f
3f5b774
d5de17b
669277b
834d78d
dadf2ff
9c7e7b5
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -254,16 +254,64 @@ final record: | |
## Merging record with metadata | ||
|
||
Metadata can be attached to values thanks to the `|` operator. Metadata | ||
currently includes contract annotations, default value, and documentation. We | ||
describe in this section how metadata interacts with merging. | ||
currently includes contract annotations, default value, merge priority, and | ||
documentation. We describe in this section how metadata interacts with merging. | ||
|
||
### Default values | ||
### Merge priorities | ||
|
||
Priorities are specified using the `priority` annotation, followed by a number | ||
literal. There are also two other special priorities, the bottom priority, specified | ||
using the `default` annotation, and the top priority, specified using the | ||
`force` annotation. | ||
|
||
Priorities dictate which values take precedence over other values. By default, | ||
values are given the priority `0`. Values with the same priority are recursively | ||
merged as specified in this document, which can mean failure if the values can't | ||
be meaningfully merged: | ||
|
||
```text | ||
nickel> {foo = 1} & {foo = 2} | ||
error: non mergeable terms | ||
┌─ repl-input-1:1:8 | ||
│ | ||
1 │ {foo = 1} & {foo = 2} | ||
│ ^ ^ with this expression | ||
│ │ | ||
│ cannot merge this expression | ||
│ | ||
= Both values have the same merge priority but they can't be combined | ||
``` | ||
|
||
On the other hand, if the priorities differ, the value with highest priority | ||
simply erases the other in the final result: | ||
|
||
```text | ||
nickel> {foo | priority 1 = 1} & {foo = 2} | ||
{ foo = 1 } | ||
|
||
nickel> {foo | priority -1 = 1} & {foo = 2} | ||
{ foo = 2 } | ||
``` | ||
|
||
The priorities are ordered in the following way: | ||
|
||
- bottom is the lowest priority | ||
- numeral priorities are ordered as usual numbers (priorities can be any valid Nickel | ||
number, including fractions and negative values) | ||
- top is the highest priority | ||
|
||
#### Default values | ||
|
||
A `default` annotation can be used to provide a base value, but let it be | ||
overridable through merging. For example, `{foo | default = 1} & {foo = 2}` | ||
evaluates to `{foo = 2}`. Without the default value, this merge would have | ||
failed with a `non mergeable fields` error, because merging being symmetric, it | ||
doesn't know how to combine `1` and `2` in a generic and meaningful way. | ||
evaluates to `{foo = 2}`. A default value is just a special case of a priority, | ||
being the lowest possible one. | ||
|
||
#### Forcing values | ||
|
||
Dually, values with the `force` annotation are given the highest priority. Such | ||
a value can never be overriden, and will either take precedence over another | ||
value or be tentatively merged if the other value is forcing as well. | ||
Comment on lines
+312
to
+314
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. same idea as default but with |
||
|
||
#### Specification | ||
|
||
|
@@ -284,10 +332,6 @@ same on both side: | |
} | ||
``` | ||
|
||
Currently, there are only two priorities, `normal` (by default, when nothing is | ||
specified) and the `default` one, with `default < normal`. We plan to add more | ||
in the future (see [RFC001](https://github.com/tweag/nickel/blob/c21cf280dc610821fceed4c2caafedb60ce7177c/rfcs/001-overriding.md#priorities)). | ||
|
||
#### Example | ||
|
||
Let us stick to our firewall example. Thanks to default values, we set the most | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
# Merge priorities | ||
|
||
This example is a variant of the merge example that makes use of merge | ||
priorities. The code to run lies in `main.ncl`. The default value | ||
`firewall.enabled` defined in `security.ncl` is overwritten in the final | ||
configuration. | ||
|
||
## Run | ||
|
||
```console | ||
nickel -f main.ncl export | ||
``` |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
# Merge several blocks into one final configuration. In a real world case, one | ||
# would also want contracts to validate the shape of the data. | ||
let server = import "server.ncl" in | ||
let security = import "security.ncl" in | ||
# Disabling firewall in the final result | ||
server & security & { | ||
# As opposed to the simple merge example, this would now fail | ||
# firewall.enabled = false | ||
|
||
firewall.open_ports | priority 10 = [80], | ||
firewall.type = "superiptables", | ||
server.host.ip = "89.22.11.01", | ||
|
||
# this will only be selected if no values with higher priority (or no priority | ||
# annotation, which is the same as priority 0) is ever defined | ||
# because there's a definite value below, this won't be selected. | ||
server.host.name | priority -1 = "hello-world.backup.com", | ||
} & { | ||
server.host.name = "hello-world.main.com", | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,8 @@ | ||
{ | ||
server.host.options | priority 10 = "TLS", | ||
|
||
# force make it impossible to override this value with false | ||
firewall.enabled | force = true, | ||
firewall.type | default = "iptables", | ||
firewall.open_ports | priority 5 = [21, 80, 443], | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,5 @@ | ||
{ | ||
server.host.ip | default = "182.168.1.1", | ||
server.host.port | default = 80, | ||
server.host.name | default = "hello-world.net", | ||
} |
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -1116,7 +1116,10 @@ impl ToDiagnostic<FileId> for EvalError { | |||||
|
||||||
vec![Diagnostic::error() | ||||||
.with_message("non mergeable terms") | ||||||
.with_labels(labels)] | ||||||
.with_labels(labels) | ||||||
.with_notes(vec![String::from( | ||||||
"Both values have the same merge priority but they can't be combined", | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would keep the |
||||||
)])] | ||||||
} | ||||||
EvalError::UnboundIdentifier(ident, span_opt) => vec![Diagnostic::error() | ||||||
.with_message("unbound identifier") | ||||||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -211,7 +211,7 @@ impl UniRecord { | |
types: Some(ctrt), | ||
contracts, | ||
opt: false, | ||
priority: MergePriority::Normal, | ||
priority: MergePriority::Neutral, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. An alternative could be to use |
||
value: None, | ||
}) if contracts.is_empty() => Ok(Types(AbsType::RowExtend( | ||
id, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -235,18 +235,19 @@ fn write_query_result_<R: QueryPrinter>( | |
|
||
match &meta { | ||
MetaValue { | ||
priority: MergePriority::Default, | ||
priority: MergePriority::Bottom, | ||
value: Some(t), | ||
.. | ||
} if selected_attrs.default => { | ||
renderer.write_metadata(out, "default", &t.as_ref().shallow_repr())?; | ||
found = true; | ||
} | ||
MetaValue { | ||
priority: MergePriority::Normal, | ||
priority: MergePriority::Numeral(n), | ||
value: Some(t), | ||
.. | ||
} if selected_attrs.value => { | ||
renderer.write_metadata(out, "priority", &format!("{}", n))?; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. an improvement could be to check 0 equality and don't print anything if it's the case There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not really, because |
||
renderer.write_metadata(out, "value", &t.as_ref().shallow_repr())?; | ||
found = true; | ||
} | ||
|
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.
Even if the implementationementation doesn't manage as such, may we say it is like having
-inf
value? Actually, how nickel manage infinites?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.
Infinite can't be represented as a float literal (they can be obtained by operations on float, I guess, but not written down), so
+inf
and-inf
aren't currently possible as numeral priorities. I don't know, is it really that clearer to say that it is-inf
, rather than spelling out loud "this is the lowest possible priority"? To me the latter requires less maths notion (arguably,+inf
and-inf
are not exactly rocket science, but still). I don't have a strong opinion, honestly.