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 mkString to HList and tuples #148

Merged
merged 1 commit into from Jul 1, 2014
Merged

Conversation

stacycurl
Copy link
Collaborator

No description provided.

@@ -811,18 +811,17 @@ object tuple {
*
* @author Miles Sabin
*/
trait ToList[T, Lub] extends DepFn1[T]
Copy link
Owner

Choose a reason for hiding this comment

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

This doesn't look quite right ... why is there ToList-related stuff in this PR?

@milessabin
Copy link
Owner

Aside from the stray ToList stuff that's got itself into the mix this looks fine ... I'll merge when you've sorted that out. Thanks :-)

@stacycurl
Copy link
Collaborator Author

The ToList for tuples had an unconstrained output type, unlike that for hlists which states that the result is a List[Lub]. The implementation relies on converting to a List before calling mkString on it, I can't call mkString on an arbitrary type.

Of course I could add a MkString type class for tuples but I figured that the return type for ToList should be List[Lub].

@milessabin
Copy link
Owner

OK, gotcha ... I'm going to hold off on this until #119 is merged.

@milessabin
Copy link
Owner

Could you rebase this now that #119 has been merged?

@milessabin milessabin mentioned this pull request Jun 30, 2014
@milessabin
Copy link
Owner

Whoops ... it was #114 I merged, not #119 ... still pending.

@milessabin
Copy link
Owner

I think that #119 should subsume your ToList change.

As an aside, I would prefer that type class signature to have been written as,

trait ToList[T, Lub] extends DepFn1[T] { type Out = List[Lub] }

@milessabin
Copy link
Owner

Could you update this now that #119 has been merged? :-)

@stacycurl
Copy link
Collaborator Author

Updated

@milessabin
Copy link
Owner

Cool ... I'm not persuaded by the defaulted variants though. I think .mkString() should equal .toString (which makes it fairly useless) and that if you want to specify any of the prefix, separator and suffix you'll want to specify them all (especially given the choice of "⸨" and "⸩" as the default prefix and suffix ;-)

@stacycurl
Copy link
Collaborator Author

Grr, updated. It's a simpler commit now but I still think ⸨⸩ are brilliant delimiters for a hlist, like a tuple but more so !

I had the 3 variants to mimic those on Traversable.

@milessabin
Copy link
Owner

Less is more ;-)

Merging now ... many thanks ... much appreciated :-)

milessabin added a commit that referenced this pull request Jul 1, 2014
Add mkString to HList and tuples
@milessabin milessabin merged commit 941a31a into milessabin:master Jul 1, 2014
@kevinwright
Copy link
Collaborator

Don't worry stacy... I once tried to use «» for the interpolation in boilerplate, and was told it was "too cute". You're not the only person to experience unicodophobia here :)

@milessabin
Copy link
Owner

:D

@stacycurl stacycurl deleted the mkString branch July 1, 2014 23:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants