Implement sequential[B](tasks: Seq[Initialize[Task[B]]]) and remove useless coment outs

[3tty0n]

This implements sequental[B](tasks: Seq[Initialize[Task[B]]]) method in TaskSequential.scala and remove useless comment outs.

The return value of sequential[B](tasks: Seq[Initialize[Task[B]]]) is the last one of tasks.

Before:

lazy val tasks = Seq(taskA, taskB, taskC)
test in Test := Def.sequential(tasks.init, tasks.last).value // a little confusing

After:

lazy val tasks = Seq(taskA, taskB, taskC)
test in Test := Def.sequential(tasks).value // looks nice

[jvican]

Hey @3tty0n, tihs looks great! I've always wondered why it wasn't easier to do this. LGTM.

Thanks for taking the time to report the issue and address it in a PR. Small changes matter 😄.

[jvican]

Extra 'p' here 😄!

[3tty0n]

Thank you for your quick reply!
I'll fix it as soon as possible.

[3tty0n]

I fixed the typo in the notes, but the CI was failed (even though the previous CI test was succeessful). Should I update the branch?

EDIT:
I updated this branch.

[dwijnand]

I don't like the addition of this overload because it could fail Nil. Put differently: it uses init and last which aren't partial functions.

[edit: ... which are* partial functions]

[jvican]

That makes sense. Can we special case for list of only one element and return the task itself instead of calling the underlying sequential?

[jvican]

*for one and zero elements.

[3tty0n]

I agree with this idea.

Can we special case for list of only one element and return the task itself instead of calling the underlying sequential?

[3tty0n]

I will implement and push it when I go back home.

[3tty0n]

I have one problem.
How sequential[B](tasks: Seq[Initialize[Task[B]]]) behave when tasks is Nil ?
It seems good to do nothing, but Task[Unit] is not inferred to Task[B]].

---

def sequential[B](tasks: Seq[Initialize[Task[B]]]): Initialize[Task[B]] =
  tasks.toList match {
     case Nil => ???
     case x :: Nil => Def.task { x.value }
     case _ =>
       sequential(tasks.init.map(unitTask), tasks.last)
   }

[jvican]

@3tty0n Given the tradition of sbt of throwing exceptions all over the place, I would just throw an exception if the list is zero, and document it with Scaladoc.

[ShaneDelmore]

What about using a NonEmptySeq?

[3tty0n]

@jvican Thanks. I will try it as you told me.

@ShaneDelmore NonEmptySeq is built-in data structure? If not, I think using NonEmptySeq is not recommended.

[dwijnand]

Let's not add partial methods to the API.

[jvican]

The full of sbt API, both public and internal, is full of them. I see why we shouldn't, honestly.

[jvican]

Okay, I agree this is far than ideal. What if we just return an empty task if the argument of sequential is empty? Then we're total. We should document it well in the public API. Are you happier with this @dwijnand?

[dwijnand]

What's an empty task? Given 0 tasks of B how are you going to return 1 task of B?

[jvican]

Got confused by the unitTask in SequentialTask. I see two ways to move this forward:

1. Use https://github.com/tarao/nonempty-scala. Instead of List(1,2,3) we can do NonEmpty(1,2,3) and ensure that the collection is non empty. Pretty neat. The idea would be to port it to sbt/sbt.
2. Require ::. The major disadvantage with this approach is that 1::Nil returns List not ::. So users of the sequential function asking for a list would need to patmat on the list before calling the method.

IMO, the first one is the best. I can see NonEmpty being useful in other places of the sbt API. I think it's a good idea to port it.

[jvican]

@3tty0n I think the best solution is to port https://github.com/tarao/nonempty-scala to sbt. It would be a very meaningful addition because it would allow us to enforce better the contract of the APIs and ensuring that collections are non-empty by design. Then, we can use it in the following way:

lazy val tasks = NonEmpty(taskA, taskB, taskC)
test in Test := Def.sequential(tasks).value

/cc @eed3si9n

[3tty0n]

@jvican Thanks. I think porting and using NonEmpty is good way to realize the idea of this PR.

[eed3si9n]

Looking at this now, I kind of feel like adding NonEmptyList just for this sounds like a maintenance burden. Is Def.sequential(tasks.init, tasks.last).value that bad?

[eed3si9n]

Closing this in favor of #4369