Open roberth opened 1 year ago
Not a fan of this, because:
Builtins don't need to have a good user interface, they just need to provide the primitive to allow doing basic operations efficiently. Nixpkgs' lib
is a much better place for this.
I'd also rather avoid functions that accept multiple types, because it makes type annotations non-trivial and harder to understand:
removeAttrs :: (List | Attrs) -> Attrs -> Attrs
# Or:
removeAttrs :: HasKeys ks => ks -> Attrs -> Attrs
In general it sounds like a design smell for functions to only use parts of their arguments. This leads to surprises, because users expect changes to arguments cause changes to the result.
E.g. one might think that removeAttrs { a = 10; } { a = 10; b = 20; } == { b = 20; }
compares the values of a
to determine whether it should be removed, leading to surprises when changing the first a
doesn't change the result.
It's not obvious which attributes removeAttrs fooAttrs barAttrs
persist and which ones does it removes. This is worsened by intersectAttrs fooAttrs barAttrs
working exactly the opposite way.
Imo the path forward is to leave the builtins alone and instead implement better functions in lib
, e.g.:
lib.attrsets.removeKeys :: List -> Attrs -> Attrs
lib.attrsets.selectKeys :: List -> Attrs -> Attrs
Such that a == lib.attrsets.unionOfDisjoint (removeKeys k a) (selectKeys k a)
.
- Builtins don't need to have a good user interface
True, but implemented in C++ would perform slightly better.
- In general it sounds like a design smell for functions to only use parts of their arguments.
Should be somewhat expected in a lazy language, but I'm biased; I'm rather comfortable with laziness. That's rare.
removeKeys
We never use "keys". It's always something like attributes, attrNames, attrs.
List ->
Conversions to list tend to allocate more, and the algorithms can't take advantage of the list's properties, because it has basically none.
The performance argument is a really good one.
Here's another thought: These two operations (selecting attributes and removing attributes) can probably be done more efficiently together (which is often also what you actually need). So how about something like this:
builtins.diffAttrs { a = 0; b = 0; } { b = 1; c = 1; }
-> {
only.left = { a = 0; };
both.left = { b = 0; };
both.right = { b = 1; };
only.right = { c = 1; };
}
This does cost extra attribute allocations (though this could be optimised with an idea like https://github.com/NixOS/nix/issues/7676). However this cost is constant, compared to the cost of having to run both intersectAttrs
and removeAttrs
.
That's a lot like a zipAttrsWith
, but limited to two arguments.
Another option is
combineAttrs (name: left: left) (name: left: right: left + right) (name: right: right) { a = 0; b = 1; } { b = 10; c = 20; }
{ a = 0; b = 11; c = 20; }
or if you want more flexibility in the style of lib.concatMapAttrs
mergeZipAttrs (name: left: { ${name} = left; }) (name: left: right: { ${name} = left + right; ${name}-left = left; ${name}-right = right; }) (name: right: { ${name} = right; }) { a = 0; b = 1; } { b = 10; c = 20; }
{ a = 0; b = 11; c = 20; b-left = 1; b-right = 10; }
Idk what's better.
Is your feature request related to a problem? Please describe.
I'd expect the following to work,
but instead I have to write this:
This is arbitrary. Let's just make it work.
Describe the solution you'd like
The second argument of
removeAttrs
is allowed to be an attrset where the values are ignored.The first argument of
intersectAttrs
is allowed to be a list where the items are interpreted as attribute names.Describe alternatives you've considered
lib
.Additional context
Priorities
Add :+1: to issues you find important.