Added RestAction#onSuccess

[Zabuzard]

Pull Request Etiquette

• I have checked the PRs for upcoming features/bug fixes.
• I have read the contributing guidelines.

Changes

• Internal code
• Library interface (affecting end-user code)
• Documentation
• Other: _____

Closes Issue: NaN

Description

Added two one new user-facing convenience method on RestAction, accept and also onSuccess. It represents a frequent scenario on action-chaining that otherwise requires cumbersome workarounds for the user.

onSuccess

The method works like map, but results in a RestAction<T>, continuing with the previous result. It just consumes the given result, but allows continuation of the action-chain:

guild.retrieveMemberById(userId) // RestAction<Member>
  .onSuccess(System.out::println)   // RestAction<Member>
  .flatMap(any -> channel.sendMessage("Done"))
  .queue()

This enables users to chain in void methods.

The workaround without this helper would be:

guild.retrieveMemberById(userId)
  .map(member -> {
    System.out.println(member);
    return member;
  })
  .flatMap(any -> channel.sendMessage("Done"))
  .queue()

Which works well, but is just annoying boilerplate. In particular since method references can not be used.

The name onSuccess is inspired by similar methods in JDA. For example the callback in queue(onSuccess) or Task#onSuccess. Also, similar to onErrorMap and onErrorFlatMap.

accept

(Removed in favor of onSuccess which also handles this use case)

The method works like map, but results in a RestAction<Void>. It just consumes the given result, but allows continuation of the action-chain:

guild.retrieveMemberById(userId) // RestAction<Member>
  .accept(System.out::println)   // RestAction<Void>
  .flatMap(any -> channel.sendMessage("Done"))
  .queue()

This enables users to chain in void methods.

The workaround without this helper would be:

guild.retrieveMemberById(userId)
  .<Void>map(member -> {
    System.out.println(member);
    return null;
  })
  .flatMap(any -> channel.sendMessage("Done"))
  .queue()

The main issue for the user is that Void is obviously incompatible with void and hence can not be automatically inferred. Resulting in a lot of boilerplate for the user.

The name accept is inspired by Javas future API: CompletableFuture#thenAccept

also

(Removed in favor of onSuccess which also handles this use case)

This method is quite similar to accept, but instead it forwards the previous result, continuing with RestAction<T>:

guild.retrieveMemberById(userId)  // RestAction<Member>
  .also(System.out::println)      // RestAction<Member>
  .flatMap(Member::openPrivateChannel)
  .flatMap(channel -> channel.sendMessage("hello"))
  .queue()

This enables users to chain in void methods.

The workaround without this helper would be:

guild.retrieveMemberById(userId)
  .map(member -> {
    System.out.println(member);
    return member;
  })
  .flatMap(Member::openPrivateChannel)
  .flatMap(channel -> channel.sendMessage("hello"))
  .queue()

A bit less boilerplate for the user, since the type can at least be automatically inferred. However, it is still annoying that method references can not be used and the user is forced to introduce lambdas.

The name also is inspired by Kotlins scoped function: .also

Discussion

An argument can be made that all users of accept could also just use also and just ignore the action result (what they have to do with Void anyways).

I.e. the previous example for accept could also be written using also:

guild.retrieveMemberById(userId) // RestAction<Member>
  .also(System.out::println)   // RestAction<Member>
  .flatMap(any -> channel.sendMessage("Done")) // just ignore the member
  .queue()

That would allow us to just remove accept, maybe renaming also into accept then 🤷 The advantage is clear:

• less confusion for the user due to less options to choose from
• less code to maintain

Personally, I am actually in favor of this decision. Let me hear your opinion on it and we can adjust the PR slightly 👌

Edits

Removed accept, kept also (because it can cover both use cases fine). But renamed also to onSuccess.

[MinnDevelopment]

I think we should use the name onSuccess which can add side-effects like this. It would simply accept Consumer<? super T>.

I don't like the name accept as its not very clear or discoverable. Also, the also should be removed since its not necessary and clashes with the kotlin function.

[Zabuzard]

I think we should use the name onSuccess which can add side-effects like this. It would simply accept Consumer<? super T>.

I don't like the name accept as its not very clear or discoverable. Also, the also should be removed since its not necessary and clashes with the kotlin function.

Didnt think about Kotlin interoperability, you are right with that - the name clashes.

I am okay with onSuccess, but I think it does not play very well along the existing naming.

Right now, RestAction reads very similar to Javas Stream API, the equivalent there would be maybe peek. For CompletableFuture, it is accept (vs apply).

Ultimately, I do not care too much about the naming and trust your experience on this. As long as the functionality is well received, I am happy 😄

[Zabuzard]

Actually, after thinking about it again, I think your name is a great idea. In particular if we think about .queue(onSuccess, onError). So the action-chain could then look like:

action.map(...)
  .onSuccess(...)
  .onSuccess(...)
  .flatMap(...)
  .onSuccess(...)
  .map(...)
  .queue();

Reads well.

[MinnDevelopment]

The reason I choose this altered name is because of two reasons:

• It only adds a side-effect (unlike map which is a transfer function)
• It is essentially an event listener (onEvent)

And I do not want to use the naming of CompletableFuture because I believe it is bad.

Another important thing to consider is whether errors should be propagated or caught, I'm not quite sure which is better in this case.

[Zabuzard]

Agree to your points.

Another important thing to consider is whether errors should be propagated or caught, I'm not quite sure which is better in this case.

Kinda depends on the naming. With my initial proposals, I think the user would expect it to propagate, since its part of the chain

With onSuccess however, I can see your point, as it is really just a side-effect and not necessarily "part of the chain".

[Zabuzard]

Suppressing exceptions is dangerous though. It can lead to a lot of confusion for users. I would vote for propagating exceptions and let the user decide. If they want to chain in a method that can throw, they can just add try-catch themselves.

If that clashes with the intention that the name suggests, we have to pick a better name.

[java-coding-prodigy]

In my opinion we should use peek as the name, inspired from Stream#peek which also applies a consumer on the data and returns the same value

[V-Play-Games]

Something like addToCallbackChain(Success, Failure) would work since it clearly states it's adding callbacks to the "callback chain"

[Zabuzard]

As a note, I admit that the use case for also is definitely to add some sort of "side-operation", but the motivation behind accept is not.

I frequently run into the situation where I want to work with the data that the rest-chain spits out, but still do not want to finish the chain and continue another operations after it. Such as:

task1.accept(...)
  .flatMap(any -> task2)
  .queue(...);

So it is similar to queue(onSuccess), with the difference that you want to continue the chain (with implicit stop on failure).

The motivation for accept is essentially the same as for map and flatMap, just that sometimes you want to use a void method. flatMap naturally is already able to handle RestAction<Void>, but map just cant. So the proposal for accept is really just about giving us a map variant that can deal with void.

---

That said, I am going to quickly push a version that only has also (since it can cover both use cases), renamed to onSuccess for the time being.

[MinnDevelopment]

just that sometimes you want to use a void method

Could you give an example use-case?

[Zabuzard]

just that sometimes you want to use a void method

Could you give an example use-case?

Looking through the code base, I found a couple like this:

So there is a method that is void and its clearly part of the action chain. Its not some "side-operation". Its supposed to partipate in the chain. But at the same time, the chain is not at an end yet and is supposed to continue (so queue(onSuccess) doesnt do the trick).

So the input to the map has to be routed around the call, so that it acts as the output again.

In my opinion, the motivation is the same as for .map or .flatMap. You want to execute some operation on the result and then continue the chain with more. Only difference is that in this case you do not add any new data-transformation to it, i.e. void.

---

I also found a second use case in our code base, that might be in the need for a helper:

Here, one wants to flatMap something that is RestAction<Void>, but instead of having it Void, it should reuse the existing payload instead. So this would be some sort of RestAction<T> onSuccess(Function<T, RestAction<Void>>) variant.

A bit niche though, maybe not in the need for a general purpose method - unless others experienced this use case as well.

[MinnDevelopment]

So there is a method that is void and its clearly part of the action chain. Its not some "side-operation". Its supposed to partipate in the chain. But at the same time, the chain is not at an end yet and is supposed to continue (so queue(onSuccess) doesnt do the trick).

To me, it seems like onSuccess can be used for this, since that is exactly how it works. I don't really understand the problem here.

Here, one wants to flatMap something that is RestAction, but instead of having it Void, it should reuse the existing payload instead. So this would be some sort of RestAction onSuccess(Function<T, RestAction>) variant.

I definitely don't think this is something we should have.

[Zabuzard]

To me, it seems like onSuccess can be used for this, since that is exactly how it works. I don't really understand the problem here.

I dont quite follow. Such a method does not exist on RestAction yet. How would you implement above without the utility added in this PR?

The chain is:

• DM user (RestAction)
• warn user (void)
• send feedback (map to MessageEmbed)
• reply embeds (flatMap with a RestAction)

in that order. And any failure should cause the whole chain to fail.

Also, this chain didnt end yet. It returns it as RestAction for more additions, if desired. Its not feasible to end it already.

The motivation is exactly the same as for map or flatMap. You want to add an action to the chain - with the difference that this action doesnt transform but just consume.

[Zabuzard]

Any news on this?