Skip to content

lib.attrsets.matchAttrTag: init#304328

Draft
infinisil wants to merge 1 commit intoNixOS:masterfrom
tweag:matchAttrTag
Draft

lib.attrsets.matchAttrTag: init#304328
infinisil wants to merge 1 commit intoNixOS:masterfrom
tweag:matchAttrTag

Conversation

@infinisil
Copy link
Member

@infinisil infinisil commented Apr 15, 2024

Description of changes

As suggested in #284551 (review), this is a useful utility function for working with values from lib.types.attrTag:

lib.matchAttrTag "my.option" value.my.option {
  tag1 = tagValue: "Tag1: ${tagValue}";
  tag2 = tagValue: "Tag2: ${tagValue}";
}

This was written in Nix Hour 68

Ping @roberth

Things done

  • Add documentation
  • Add tests
  • Update lib.types.attrTag docs to mention this function

Add a 👍 reaction to pull requests you find important.

@github-actions github-actions bot added the 6.topic: lib The Nixpkgs function library label Apr 15, 2024
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Apr 15, 2024
n = number: "It's the number ${toString number}";
s = str: "It's the string ${str}";
}
=> error: test: No handler for tag "unknown"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"test" doesn't seem like a great example. It'd help to have a more realistic use case. When it's a realistic example, it's easier to see what's expected from the context.

When this error occurs in the context of a module that defines an option of type attrTag ..., this case will only occur due to programming error, and I don't think the module author should have the foresight to clarify that in the context.

In other cases, you probably do want the context to be quite general.
Maybe we should have another function that takes an option instead of its value? Doesn't work for attrTags when used as part of type constructors like attrsOf though.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What if the merged result of attrTag is the matching function itself 🤔

config.some.option {
  n = number: "It's the number ${toString number}";
  s = str: "It's the string ${str}";
}

This would be doable effectively in a backwards-compatible way using __functor! And this way we should be able to get a good error message for free. Not a big fan of __functor in general, but in this case I'm really wondering..

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For my use case, settings, I need to not have it:

       … while evaluating attribute '__functor'

error: cannot convert a function to JSON

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess it also wouldn't be very backwards compatible, because any code that asserts for the attribute set to only have one attribute would fail.

:::
*/
matchAttrTag =
context: value: handlers:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
context: value: handlers:
context: handlers: value:

Often, the handlers are more part of the program than part of the data, so arranging them like this lets us take advantage of currying, in things like map, pipes, or eta-reduced function definitions.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the most common case is to not curry tbh, and you can still use flip if you need it. With the value: handlers: order we're also mirroring how matching looks like in many other languages:

match value {
  Foo { .. } => 0
  Bar { .. } => 1
}
case value of
  Foo { .. } -> 0
  Bar { .. } -> 1

So I'd really prefer leaving it like that

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The annoying thing is that if you do want to lib.flip, you have to write it like flip (matchAttrTag "context") [...] which looks a bit funny to me

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess if we go for #304328 (comment) this won't be a problem anymore :D

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pipe friendly:

map (matchAttrTag "context") myList

and

value |> matchAttrTag "context" {
  ...
}

Not pipe friendly:

map (flip (matchAttrTag "context")) myList

and

matchAttrTag "context" value {
  ...
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also, fwiw, avoiding flip avoids a runtime cost.

@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label May 3, 2024
@pluiedev
Copy link
Member

Oh this looks really nice. I make extensive use of attrTag and this would make the processing logic much easier (at least, it would be much simpler than whatever this is 😅)

@wegank wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jan 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

2.status: merge conflict This PR has merge conflicts with the target branch 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 6.topic: lib The Nixpkgs function library 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants