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 ResultFailureException and T.fail convenience API #3422

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

lefou
Copy link
Member

@lefou lefou commented Aug 26, 2024

The T.fail shortcut is an experiment. Pease comment WDYT.

This change was discussed in #3406

I'd also like to remove the Exception from Result.Failing hierarchy,
but it was added before the last stable release, so this would be
considered a binary incompatible change.

Removed `Exception` from `Result.Failing` hierarchy.
It was added after the last release, so this should be
considered a binary compatible change.

The `T.fail` shortcut is an experiment. Pease comment WDYT.

This change was discussed in #3406
It needs to be removed later, when we can break bin-compat.
@lefou
Copy link
Member Author

lefou commented Aug 28, 2024

The test covering this task:

    def sometimesFailingWithException(fail: Boolean): Task[String] = T.task {
      if (!fail) "Success"
      else T.fail("Failure")
    }

fails with

X mill.eval.SeqTaskTests.sometimeFailingWithException.failure 2ms 
  java.lang.AssertionError: assertion failed: ==> assertion failed: Left(mill.api.ResultFailureException: Failure
      mill.api.Ctx$Fail.fail(Ctx.scala:109)
      mill.api.Ctx$Fail.fail$(Ctx.scala:109)
      mill.api.Ctx.fail(Ctx.scala:124)
      mill.define.Target$.fail(Task.scala:199)
      mill.eval.TaskTests$Build.$anonfun$sometimesFailingWithException$1(TaskTests.scala:129)
      mill.define.Task$TraverseCtx.evaluate(Task.scala:71)) != Left(mill.api.Result$Failure)
    scala.Predef$.assert(Predef.scala:279)
    utest.asserts.Asserts$ArrowAssert.$eq$eq$greater(Asserts.scala:90)
    mill.eval.TaskTests.$anonfun$tests$44(TaskTests.scala:285)
    mill.eval.TaskTests.$anonfun$tests$44$adapted(TaskTests.scala:284)
    mill.eval.SeqTaskTests$.withEnv(TaskTests.scala:296)
    mill.eval.TaskTests.$anonfun$tests$43(TaskTests.scala:149)

So, I wonder why?

The T.task macro expects a Result[T], but I return a not-Result-type, so I think the compiler should apply the implicit Result.create method, which surrounds the t: => T with a try-catch-block to lift the ResultFailureException thrown by T.fail into a Result.Failure. Yet, it's obviously not working as I think it should. Any ideas?

@lefou lefou requested a review from lihaoyi August 28, 2024 16:13
@lolgab
Copy link
Member

lolgab commented Aug 28, 2024

Instead of converting to a Result.Failure you could add a rule to pretty print the error message as I did in the other PR for Result.Exception(Result.Failure) but for Result.Exception(ResultFailureException)
What do you think?

@lefou
Copy link
Member Author

lefou commented Aug 28, 2024

Instead of converting to a Result.Failure you could add a rule to pretty print the error message as I did in the other PR for Result.Exception(Result.Failure) but for Result.Exception(ResultFailureException) What do you think?

The idea is, to keep the control flow of tasks returning results functional. I still have this idea in my mind, that we one day can recover from failed tasks (#1779), but that's only possible if we stick to a uniform way of failing.

@lihaoyi
Copy link
Member

lihaoyi commented Sep 2, 2024

I think this approach looks fine by me

@lefou
Copy link
Member Author

lefou commented Sep 8, 2024

This PR is still in draft state, since there is one open issue: #3422 (comment). Has an fix or an idea?

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