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

Override Failure.toString implementation #3386

Closed
wants to merge 1 commit into from

Conversation

jilen
Copy link
Contributor

@jilen jilen commented Aug 19, 2024

Avoid the very confusing failure message like

1 targets failed
ws.resolvedIvyDeps mill.api.Result$Failure
    mill.api.Result$Failure.map(Result.scala:75)
    mill.api.Result$Failure.map(Result.scala:74)
    mill.scalalib.Lib$.resolveDependencies(Lib.scala:77)
    mill.scalalib.CoursierModule$Resolver.resolveDeps(CoursierModule.scala:146)
    mill.scalalib.JavaModule.$anonfun$resolvedIvyDeps$3(JavaModule.scala:535)

@lolgab
Copy link
Member

lolgab commented Aug 19, 2024

I don't understand what happened between Mill 0.11.10 and 0.11.11 which broke the error messages.

@lihaoyi
Copy link
Member

lihaoyi commented Aug 19, 2024

Probably was broken by #3330

@lolgab
Copy link
Member

lolgab commented Aug 19, 2024

getOrThrow is a lossy conversion in case of Failure. Isn't it better to return Result[Agg[PathRef]] in resolveDeps?

@lihaoyi
Copy link
Member

lihaoyi commented Aug 19, 2024

Possibly; throwing exceptions is quick and dirty, but the current loss of error message is just a bug AFAICT and isn't fundamental to the question of exception vs result. I thought I propagated the error message in that PR, but clearly i missed something.

We could add an overload to return a Result, but I think that's orthogonal to fixing the issue at hand

@lolgab
Copy link
Member

lolgab commented Aug 19, 2024

The bug should be fixed in #3391.
Nevertheless, we can improve the toString implementation so that in the rare case that someone calls .toString, the result has meaning. It can also be "Result.Failure($msg, $value)"

@lefou
Copy link
Member

lefou commented Aug 19, 2024

I think the provided patch won't improve the error message presented in the initial comment. In contrast to the merged PR #3391. We should keep the default toString of the case class.

@lihaoyi lihaoyi closed this Aug 19, 2024
@jilen
Copy link
Contributor Author

jilen commented Aug 20, 2024

I think the provided patch won't improve the error message presented in the initial comment. In contrast to the merged PR #3391. We should keep the default toString of the case class.

The toString already overrided by Failing trait. Override toString actually ensure the msg not lost at every corner case.

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