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

Conversions between nested pairs and HLists #701

Merged
merged 3 commits into from
Mar 18, 2020

Conversation

mgzuber
Copy link
Contributor

@mgzuber mgzuber commented Mar 21, 2017

No description provided.

@mgzuber mgzuber force-pushed the topic/NestedPairs branch from 7d35d9e to 05dccef Compare March 21, 2017 13:53
@codecov-io
Copy link

codecov-io commented Mar 21, 2017

Codecov Report

Merging #701 into master will increase coverage by 0.07%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #701      +/-   ##
==========================================
+ Coverage   87.99%   88.07%   +0.07%     
==========================================
  Files          64       64              
  Lines        1499     1509      +10     
  Branches        7        7              
==========================================
+ Hits         1319     1329      +10     
  Misses        180      180
Impacted Files Coverage Δ
...e/src/main/scala/shapeless/syntax/std/tuples.scala 83.72% <100%> (+0.19%) ⬆️
core/src/main/scala/shapeless/syntax/hlists.scala 90.07% <100%> (+0.07%) ⬆️
core/src/main/scala/shapeless/ops/hlists.scala 93.44% <100%> (+0.12%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b141d0c...23e4492. Read the comment docs.

@mgzuber
Copy link
Contributor Author

mgzuber commented Apr 28, 2017

@milessabin Any comments either way?

@milessabin
Copy link
Owner

Yes, this looks useful. The corresponding definitions for nested Either to `Coproducts would also be useful. I think there are a couple of issue which need to be resolved though.

First you've reused a lot of existing names (eg. ToHList) and added names which are very close to existing names (eg. toTuple vs. tupled). Can you come up with something which distinguishes your additions from what's already there and more clearly illustrates the different semantics?

Second, I'm not completely convinced at the way the nested pairs are terminated ... why not have () as the rightmost element, analogous to HNil?

@milessabin
Copy link
Owner

I'll consider merging this if you rebase and address the issues raised in my previous comment.

@milessabin milessabin added this to the shapeless-2.4.0 milestone Dec 18, 2017
@mgzuber
Copy link
Contributor Author

mgzuber commented Feb 14, 2018

My apologies on having been MIA.

To address your above comments.

  • The definitions for nested Either to Coproduct was already merged in Conversions between Coproducts and Eithers #699.
  • I couldn't find a similar name to toHList in the tuple syntax. I'm of the opinion that this is a reasonable name, but am more than happy to change it if you still disagree
  • I changed toTuple to nestedProduct
  • Your second point is one of preference I think. I thought it convenient to be able to convert a 2-tuple directly to a HList and a length 2 HList directly to a 2-tuple. However, I don't have a strong opinion on this, so I've changed the behaviour to be as you described.

Let me know if you'd like more changes, I will get to them quickly!

To be more consistent with nested `Either` to `Coproduct` conversions.
@joroKr21 joroKr21 merged commit 0495f42 into milessabin:master Mar 18, 2020
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.

4 participants