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

add CoprodSubsetter for splitting coproducts #89

Merged
merged 2 commits into from
Mar 21, 2018

Conversation

ExpHP
Copy link
Collaborator

@ExpHP ExpHP commented Mar 20, 2018

Part of #86.

  • Adds CoprodSubsetter with a subset method to split a coproduct. The idea behind the name is that you are handling a subset of the types, leaving the rest for somebody else to handle.

  • Added an example of using CoprodInjector for a match that is actually exhaustive. It didn't seem right for only CoprodSubsetter to have such an example when CoprodInjector has the word "exhaustive" right there in the headline.

/// # fn main() {
/// type I32F32 = Coprod!(i32, f32);
///
/// // be aware that this particular example could be
Copy link
Collaborator

Choose a reason for hiding this comment

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

s/be/Be

/// assert_eq!(handle_anything(Anything::inject(4)), "....");
/// # }
/// ```
pub trait CoproductSubsetter<Targets, Indices>: Sized {
Copy link
Collaborator

Choose a reason for hiding this comment

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

I wonder if this needs a separate trait or if it would be possible to simply do this as impls of CoproductUninjector with a label type?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

What do you mean by label type? In general I am fearful of having these be the same method (especially if it is entirely driven by type inference) because of the ambiguities created when you have a coproduct containing coproducts.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Copy link
Collaborator Author

@ExpHP ExpHP Mar 20, 2018

Choose a reason for hiding this comment

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

@Centril That is the most criminally underdocumented and bizarre feature I have ever seen.

(Well, second most bizarre. The most bizarre feature I've seen are the impls of From and Into for tuples of the wrong size, sitting there right next to the impls for tuples of matching size)

The documentation of lift_from shows HLists being produced from single values.

  • Why on earth is this same function also used to generate an HList from a prefix?
  • Why a prefix and not a suffix? Why not an arbitrarily ordered subset?
  • It shares my aforementioned concern about ambiguities.

Copy link
Collaborator

Choose a reason for hiding this comment

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

That is the most criminally underdocumented and bizarre feature I have ever seen.

Feel free to improve the documentation. The docs for LiftInto shows Hlists being produced from multiple values, but its out of sync with LiftFrom.

Why a prefix and not a suffix? Why not an arbitrarily ordered subset?

Prefixes are supported afaik.
Arbitrarily ordered subsets never crossed my mind; If you can improve on that, it would be nice.

@Centril
Copy link
Collaborator

Centril commented Mar 20, 2018

LGTM in general 👍

Copy link
Owner

@lloydmeta lloydmeta left a comment

Choose a reason for hiding this comment

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

LGTM :)

<3 the extra fixes to docs/tests

@lloydmeta lloydmeta merged commit 6917e58 into lloydmeta:master Mar 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants