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

Implement a helper for Iterator[Box[T]] => Box[List[T]] #1525

Merged
merged 3 commits into from Apr 8, 2014

Conversation

Projects
None yet
4 participants
@farmdawgnation
Copy link
Member

farmdawgnation commented Apr 4, 2014

So, I've run into cases before where I have a for comprehension I need to execute that returns an Iterator[Box[T]], but the return type of the method I'm building is Box[T]. When I'm in this situation, the following is true:

  • I want a Full[List[T]] in the event that all Boxes are full.
  • I want a ParamFailure[List[Box[T]]] in the event that one or more Failure existed in the Iterator[Box[T]].

@farmdawgnation farmdawgnation self-assigned this Apr 2, 2014

Implement toSingleBox.
The toSingleBox method takes a List[Box[T]] and turns it into a
Box[List[T]], via an implicit conversion to ListOfBoxes.
@farmdawgnation

This comment has been minimized.

Copy link
Member Author

farmdawgnation commented Apr 4, 2014

Alright folks, I provided an implementation of toSingleBox that does this. Would love some code review, and after that it'd be nice if we could spin a 2.6-M3 build. We've got quite a bit done since M2.

@farmdawgnation farmdawgnation added this to the 2.6-M3 milestone Apr 4, 2014

@japgolly

This comment has been minimized.

Copy link
Contributor

japgolly commented Apr 4, 2014

Seeing as Scalaz is already on the classpath, we can go from List[Box[T]] to Box[List[T]] for free just using sequence.

def sequence[G[_]:Applicative,A](fga: F[G[A]]): G[F[A]] =

https://github.com/scalaz/scalaz/blob/scalaz-seven/core/src/main/scala/scalaz/Traverse.scala#L100

If it doesn't exist already, I'd suggest providing an implicit for Box for its relevant properties (Applicative, Monoid, Foldable, etc).

@farmdawgnation

This comment has been minimized.

Copy link
Member Author

farmdawgnation commented Apr 7, 2014

I'm not sure you're correct about Scalaz being on the classpath. I just double checked and lift-common, the subproject Box lives in, does not seem to depend on Scalaz. And, to be honest, I think it's best that it doesn't. Thanks for the suggestion though. :)

@japgolly

This comment has been minimized.

Copy link
Contributor

japgolly commented Apr 7, 2014

Oh, it doesn't? Sorry, my bad. :)

@japgolly

This comment has been minimized.

Copy link
Contributor

japgolly commented Apr 7, 2014

That being said though, if you're happy to have lift-common depend on Scalaz, I'd be more than happy to write up the implicit instance so all the applicable Scalaz methods like sequence etc., worked on it. Just give me a shout if you change your mind.

@farmdawgnation

This comment has been minimized.

Copy link
Member Author

farmdawgnation commented Apr 7, 2014

Yeah, let's leave lift-common doing its own thing for now. If a clear benefit becomes available later, then we can discuss it on the list, but I don't think there's one now.

@Shadowfiend

This comment has been minimized.

Copy link
Member

Shadowfiend commented Apr 7, 2014

There may be space for a lift-common-scalaz or somesuch package that provides compatibility. You can spin that into a library if you'd like and bring it up on the list for those who need it, if that sounds interesting to you. That's the kind of thing that can be folded into the framework proper later on if everyone agrees and there's strong demand for it.

@dpp

This comment has been minimized.

Copy link
Member

dpp commented Apr 7, 2014

FWIW, When I split stuff into lift-commons, I envisioned it as a library that had no Scala dependencies, the only Java dependencies would be very popular well maintained libraries, and that lift-commons could be easily used in any Scala project.

I am very reluctant to have any mainline Lift modules depend on ScalaZ mainly because ScalaZ does not have a history of reliable build process and dealing with the Scala version fragility issue and a library that is not well known to always be building against the latest version of Scala is a serious concern for me. Note this is not a comment on the value of ScalaZ and if it were not for Scala's version fragility issue, I would not have the concern.

Full(fulls.map(_.value))
} else {
Failure(failureErrorMessage) ~> theListOfBoxes
}

This comment has been minimized.

@Shadowfiend

Shadowfiend Apr 8, 2014

Member

So it seems like we're using some superfluous assignments/variables here. How about this:

  if (theListOfBoxes.exists { case _: Failure => true }) {
    Failure(failureErrorMessage) ~> theListOfBoxes
  } else {
    theListOfBoxes.flatten
  }

Also, do we want to call out the fact that the length of the incoming list and the length of
the outgoing list may not be equal if there are Empty entries?

This comment has been minimized.

@farmdawgnation

farmdawgnation Apr 8, 2014

Author Member

I kind of think...

if (theListOfBoxes.exists(_.isInstanceOf[Failure])) {

would read better.

That said, you're right, there's room for optimization here like whoa.

This comment has been minimized.

@Shadowfiend

Shadowfiend Apr 8, 2014

Member

Yeah, I went back and forth on that. I'm good either way.

@japgolly

This comment has been minimized.

Copy link
Contributor

japgolly commented Apr 8, 2014

No prob, roger that regarding Scalaz.

* Helper class to provide an easy way for converting Lists of Boxes[T] into
* a Box of List[T].
**/
case class ListOfBoxes[T](theListOfBoxes: List[Box[T]]) {

This comment has been minimized.

@japgolly

japgolly Apr 8, 2014

Contributor

If you make this extend AnyVal it will save a runtime allocation when used as an implicit class.

This comment has been minimized.

@farmdawgnation

farmdawgnation Apr 8, 2014

Author Member

o.O How does that work? Can you link to where that behavior is documented per chance?

This comment has been minimized.

This comment has been minimized.

@farmdawgnation

farmdawgnation Apr 8, 2014

Author Member

Prior to Scala 2.10, AnyVal was a sealed trait. Beginning with Scala 2.10, however, it is possible to define a subclass of AnyVal

No dice since we have to support 2.9 dawgs. :(

This comment has been minimized.

@japgolly

japgolly Apr 8, 2014

Contributor

Argh, sorry, I forgot it was new to 2.10. Shocking.

This comment has been minimized.

@Shadowfiend

Shadowfiend Apr 8, 2014

Member

May be worth noting so you can open a follow-up PR for Lift 3, however.

This comment has been minimized.

@farmdawgnation

farmdawgnation Apr 8, 2014

Author Member

This guy with all his good ideas.

This comment has been minimized.

@japgolly

japgolly Apr 8, 2014

Contributor

Haha yeah, the rubbish bin loves all my good ideas! 😄
But yeah if Lift 3 is going to raise the bar from Scala 2.9 to 2.10 I'll happily go around updating stuff like that.

This comment has been minimized.

@farmdawgnation

farmdawgnation Apr 8, 2014

Author Member

I've already created the ticket for it. Will make it happen Real Soon Like™. Which will likely be momentarily after this is merged. :)

@farmdawgnation

This comment has been minimized.

Copy link
Member Author

farmdawgnation commented Apr 8, 2014

Ok, all comments have been addressed. Merge time maybe? :D

@Shadowfiend

This comment has been minimized.

Copy link
Member

Shadowfiend commented Apr 8, 2014

👍 I'm in.

@Shadowfiend

This comment has been minimized.

Copy link
Member

Shadowfiend commented Apr 8, 2014

Going to run tests locally and all that jazz and then merge 'er in.

@Shadowfiend

This comment has been minimized.

Copy link
Member

Shadowfiend commented Apr 8, 2014

Success is as success does. Let's do it.

Shadowfiend added a commit that referenced this pull request Apr 8, 2014

Merge pull request #1525 from lift/msf_issue_1525
Implement a helper for Iterator[Box[T]] => Box[List[T]]

@Shadowfiend Shadowfiend merged commit 6ca7a38 into master Apr 8, 2014

@Shadowfiend Shadowfiend deleted the msf_issue_1525 branch Apr 8, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment