Skip to content
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

Don't explicitely pattern-match on the various outputs #4112

Closed
wants to merge 6 commits into from

Conversation

thufschmitt
Copy link
Member

This is a refactoring on top of #4056 to remove as much as possible the pattern-matchings on the different kind of outputs that a derivation has. The idea behind that is that we generally don't care about the exact type of output that we have, we're only interested in the answer to one of the two questions: "is this a CA output?" and "Do we know the path of this output?". So I've added a few methods to DerivationOutput to query this directly without having to introspect the underlying output type and used that as much as possible.

(I've also mixed in a few unrelated minor changes, I can remove them if needed)

@Ericson2314
Copy link
Member

  • I definitely like the new helpers.
  • I'm skeptical of get_if chains as they make it hard to add new variants without breaking things.
  • I'm still going over the changes to the types.

@@ -1895,26 +1913,26 @@ static RegisterPrimOp primop_path({
An enrichment of the built-in path type, based on the attributes
present in *args*. All are optional except `path`:

- path
- path
Copy link
Member

Choose a reason for hiding this comment

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

Please keep the trailing white space, it's significant to mdbook (causing it to insert a <br/> between the first line and the rest).

Copy link
Member

Choose a reason for hiding this comment

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

Oh i was wondering what that was. Is there perhaps another way to do this? At least with Github's markdown

        - path

          The underlying path.

works

Copy link
Member

Choose a reason for hiding this comment

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

IIRC, I think the other difference is that lowdown treats the first as a definition list so it's rendered differently in manpages. Too bad Markdown doesn't have proper definition lists...

Copy link
Member

Choose a reason for hiding this comment

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

Too bad Markdown doesn't have proper definition lists...

Amen to that!

I think the other difference is that lowdown treats the first as a definition list so it's rendered differently in manpages

I wonder if they could be convinced to use a more lint-friendly method, like <!-- PRAGMA ... --> or something.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed while rebasing

C++ manages to make pattern-matching less convenient to use than a dirty
succession of `if`s, so let's use the latter
It doesn't make sense to have one without the other, so it's more
logical to query both of them at the same time (and inside a single
`std::optional` to ensure that we can't have one without the other).
@thufschmitt
Copy link
Member Author

@edolstra I've rebased on top of master to fix the conflicts introduced by #4114. Should be good now

@thufschmitt thufschmitt added the ca-derivations Derivations with content addressed outputs label Nov 16, 2020
@thufschmitt thufschmitt marked this pull request as draft December 18, 2020 15:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ca-derivations Derivations with content addressed outputs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants