[Feature] Merge priorities #829

yannham · 2022-09-13T14:44:28Z

This PR depends on #198. Implement merge priorities as of RFC001. The special operator default rec and force rec are not implemented in this PR but left as future work.

src/term.rs

doc/manual/merging.md

francois-caddet · 2022-09-15T14:41:57Z

doc/manual/merging.md

-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.


Even if the implementationementation doesn't manage as such, may we say it is like having -inf value? Actually, how nickel manage infinites?

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.

francois-caddet · 2022-09-15T14:43:14Z

doc/manual/merging.md

+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.


same idea as default but with +inf here.

examples/merge-priorities/main.ncl

francois-caddet · 2022-09-15T14:56:05Z

src/error.rs

-                    .with_labels(labels)]
+                    .with_labels(labels)
+                    .with_notes(vec![String::from(
+                        "Both values have the same merge priority but they can't be combined",


Suggested change

"Both values have the same merge priority but they can't be combined",

"Both values have the same merge priority so they can't be combined",

I would keep the but. Two values with the same priority may very well be combined, like records. Here the problem is that they have the same priority AND we don't know how to merge them.

francois-caddet · 2022-09-15T15:16:34Z

src/parser/uniterm.rs

-                                        priority: MergePriority::Normal,
+                                        priority: MergePriority::Neutral,


An alternative could be to use Option<float>.
then Bottom become Some(-float::inf()), Top become Some(float::inf()), and Neutral become None
Sorry not sure how to write infinite in Rust but that's the idea.
May easy the priority comparaisons but may be less clear in the code... Also is harder to pretty print, even it can be managed anyway.

francois-caddet · 2022-09-15T15:23:50Z

src/repl/query_print.rs

                    value: Some(t),
                    ..
                } if selected_attrs.value => {
+                    renderer.write_metadata(out, "priority", &format!("{}", n))?;


an improvement could be to check 0 equality and don't print anything if it's the case

Not really, because 0.0 may have been written down. We could check for Neutral though. On the other hand, it can be useful to know the actual priority, even if it hasn't been annotated?

src/term.rs

francois-caddet

A nice improvement for nickel with a clear implementation :)
Only a few changes proposed.
Just to be sure, are we really not needing Merge token anymore? maybe for RFC001 (custom merge functions)?

doc/manual/merging.md

yannham · 2022-09-16T14:52:32Z

Just to be sure, are we really not needing Merge token anymore? maybe for RFC001 (custom merge functions)?

Ah yes, good catch. Will drop this commit.

Co-authored-by: francois-caddet <francois.caddet@icloud.com>

yannham marked this pull request as ready for review September 13, 2022 14:44

yannham mentioned this pull request Sep 13, 2022

Merge priorities #827

Closed

github-actions bot temporarily deployed to pull request September 13, 2022 14:45 Inactive

github-actions bot temporarily deployed to pull request September 13, 2022 14:48 Inactive

yannham force-pushed the feature/priorities branch from d5e62a1 to 35607f5 Compare September 14, 2022 08:49

github-actions bot temporarily deployed to pull request September 14, 2022 08:54 Inactive

yannham requested review from francois-caddet and Acaccia September 14, 2022 09:31

yannham commented Sep 14, 2022

View reviewed changes

src/term.rs Outdated Show resolved Hide resolved

github-actions bot temporarily deployed to pull request September 14, 2022 13:26 Inactive

Base automatically changed from refator/tests-lib-ncl to master September 15, 2022 10:52