Refactor throwError to orOnErrorThrow

[marcindabrowski]

In my opinion throwError methods suggest that the error always be thrown.
What about changing the name to orOnErrorThrow?

[sviperll]

I'm actually not immediately convinced that this is a required change:

1. throwError name was somewhat internally consistent with mapError, discardError, recoverError.
2. Have you experienced confusion with throwError yourself or with your colleagues? For now it may look like maybe it's good to avoid confusion, but the change may not be worth it, maybe it's better to record this in some kind of idea list for version 2.0...
3. orElseThrow may be the source of confusion because it may seem that this is optional, but it is not (not sure if this is a strong argument against orElseThrow though).

Actually my story was that this method originally was orElseThrow, but after a real-world experience, I've found that we are getting much more helpers than Optional and it may be better to have some consistent naming, that's why I've switched to {map,discard,recover,throw}Error as method names. And I haven't found it confusing, there is no widely used throwError method in any other class, so it's easy to learn what it does once.

[marcindabrowski]

Well, to be honest I don't see synergy between map, recover and discard, throw.
Why? The last two are terminal operations, so they should distinct somehow from the ones that are just transforming object.
Maybe then it should be orElseThrowError?

When I was looking for the Result implementation I found these libraries.

• dichotomy
• expressible
• result

Maybe we can inspire from them in picking the right name?

[sviperll]

Maybe we can inspire from them in picking the right name?

• dichotomy and result doesn't have orElseThrow-like function at all, their philosophy is that you should give up on exceptions and always ever use errors-like-values approach
• expressible calls it orThrow

Not that great source of inspiration... WDYT about onErrorThrow?

[marcindabrowski]

Hmm. Again for me onErrorThrow has meaning that it in case of error will throw and exception, but in case of success it will do noting. I would stick to or prefix. Maybe the same way as it in expressible?

But to be honset, for me the most relevant name is the same as in Optional. I don't think that it is wrong that orElseThrow is already in Optional. I think that it is a plus, it has known meaning and actually behaves the same.
For me getting success value or throwing an exception mapped from error is the same as for optional.

WDYT?

[sviperll]

I don't think we've addressed the points:

1. It seems to me, like there is no much confusion in the first place. There is no widely used throwError method in any other class, so it's easy to learn what it does once. Please give me some context, how have you observed the confusion, has it affected you somehow?
2. the change may not be worth it, maybe it's better to record this in some kind of idea list for version 2.0 and do nothing for now.

If you think that the method with current name is actually confusing, then we can move forward and discuss naming, but I would post-pone it for now...

[marcindabrowski]

Well, it is not about confusion, but about DSL your library brings.
When I read

var result = Result......
return result.throwError(error -> new Exception());

I will not understand it as return me success value or throw an Exception, but rather throw me an Exception in case of error.

And you are saying that it will not be hard to learn that it means something different - agree, but why I have to learn it?
API should be self explanatory as possible, and this method is not.

But on the other hand below code seems to be easier to understand to person who is not familiar with your API.

var result = Result......
return result.orElseThrow(error -> new Exception());

It is similar to something I know form JDK, and I don't think it is wrong.
The same we could say about map function. Optional also has it, and you didn't named it in different manner.

All this is just my toughs, and you don't have to apply it. But I think they are reasonable.

[sviperll]

Ok. I think a stand-alone statement is a persuasive argument

var result = ...
// ...
return result.throwError(error -> new Exception());

when you see a stand-lone line that says throwError, then I would also expect exception being thrown unconditionally. I think I've mostly used throwError as part of some chain and there it is not that contradictory.

And additionally you seems to be pretty convinced, so lets move forward with naming.

Again for me onErrorThrow has meaning that it in case of error will throw and exception, but in case of success it will do noting

Isn't it exactly what it is? Where is a contradiction in the above description?

But to be honset, for me the most relevant name is the same as in Optional.
but why I have to learn it?
It is similar to something I know form JDK

I agree.

But I have a "but".

Sure it's good if we can get something that works the same way as something already in JDK, but Result is not Optional, it is more complicated and we should also do a good job explaining what it is. For now all methods that deal with errors have the word "error" in their name and I think it's a useful hint for newcomers to understand the API. So I would at aim to the middleground something that looks like JDK, but still has the word "error". So orElseThrowError and onErrorThrow both looks good to me, but onErrorThrow looks a little better, since it is a little shorter.

[sviperll]

Ok, I think I understood, why you don't like onError, because it doesn't carry the meaning "extract value". Like in the below snippet:

var value = result.onErrorThrow(error -> ...);

it looks confusing. Is the value the result of throwing. The word "or" works good to denote this, so...
What do you think about orOnErrorThrow?

var value = result.orOnErrorThrow(error -> ...);

This is a bit mouthful and may sound non-ideal, but it actually conveys the right meaning and is still consistent with other methods...

[marcindabrowski]

The value orOnErrorThrow seems OK! Great that we found consensus.
I've amended the PR to this change.

[sviperll]

We can not simply rename since this is a breaking change and we do not want to release 2.0 that fast after 1.0 release. Please add a deprecated throwError method, that simply calls orOnErrorThrow:

    /** Throws an exception. Deprecated, use {@link Result#orOnErrorThrow(Function)} instead. */
    @Deprecated
    default <X extends Exception> R throwError(Function<? super E, X> errorToExceptionConversion)
            throws X {
        return orOnErrorThrow(errorToExceptionConversion);
    }

[marcindabrowski]

Done.

[sviperll]

Please bump the version in pom.xml: 1.1.5-SNAPSHOT -> 1.2.0-SNAPSHOT to record that we are adding new functionality.

[marcindabrowski]

Done.

[marcindabrowski]

Applied suggested changes.

[sviperll]

Thanks for the high quality contribution!