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

Uni.invoke() (or call() or handle()) #146

Closed
gavinking opened this issue May 24, 2020 · 30 comments
Closed

Uni.invoke() (or call() or handle()) #146

gavinking opened this issue May 24, 2020 · 30 comments
Labels
enhancement New feature or request
Milestone

Comments

@gavinking
Copy link

After some experience I find I prefer the map()/flatMap() API to onItem().

But it's really annoying that there's no equivalent shortcut way to call UniOnItem.invoke(), since it's an operation I use all the time.

I would like to see invoke() added to Uni, though perhaps it would make more sense to name it handle(), since it's a sort of event handlery thing. Or call() perhaps.

@cescoffier
Copy link
Contributor

This issue came a few times. I, first, need to understand the use case, as most of the time, I've seen these methods used just for log purposes.

We want to add a log operator:

  • Uni.log(), Multi.log() - log all the events
  • Uni.onItem().log() - log item events
  • ...

The log operator can log automatically (using a predefined log statement) or be used with a callback returning the String to be logged.

If requested, the log should be able to clean up the stack trace to be meaningful.

When not used for logging, these methods are often used to implement side-effects. While they cannot be discarded, users must be a bit careful with that.

About the naming, what about onItem(Consumer<T>)? The "traditional" method is named doOnNext which does not fit well with the rest of Mutiny.

@gavinking
Copy link
Author

This isn't anything to do with logging. (For logging I would need a matching method that is called on failures.)

It's specifically in order to be able to call side-effects, which are rather hugely important when one is writing data-access code. :-) The most basic operations like persist() and delete() and flush() are all side-effects.

About the naming, what about onItem(Consumer<T>)? The "traditional" method is named doOnNext which does not fit well with the rest of Mutiny.

I want something that looks and feels visually like map() and flatMap(), because it will appear in a chain along with a bunch of map()s and flatMap()s.

@gavinking
Copy link
Author

You could call it affect(). Since it's used to cause an effect.

@cescoffier
Copy link
Contributor

I like that name!

@jponge @kenfinnigan WDYT?

@Ladicek
Copy link
Contributor

Ladicek commented May 25, 2020

Is this the same as peek?

@jponge
Copy link
Member

jponge commented May 25, 2020

affect sounds weird to me, but it's probably due to how I translate that to 🇫🇷 😉

Some random ideas:

  • do(Consumer<T> item) (e.g., (...).onNext().do(this::persist).(...)
  • perform
  • trigger (might be nice)

@gavinking
Copy link
Author

do is a keyword in Java.

@gavinking
Copy link
Author

And trigger() is almost a bit too exciting and action-packed, perhaps. ;-)

@jponge
Copy link
Member

jponge commented May 25, 2020

do is a keyword in Java.

Yeah, I thought about it over lunch 🤦

@gavinking
Copy link
Author

In fairness, it's probably the least used keyword except for goto and strictfp.

@kenfinnigan
Copy link
Collaborator

I kind of like affect().

Some other options could be alter(), perturb(), transform(). Just a few I thought of, though may induce other connotations.

@zeljkot
Copy link

zeljkot commented Jun 1, 2020

peek is already used in Java streams for that purpose. I vote for the peek.

@jponge
Copy link
Member

jponge commented Jun 1, 2020

also?

@gavinking
Copy link
Author

peek is already used in Java streams for that purpose

I dunno, "peek" sounds kinda like the opposite of what this is. "Peek" sounds like something with no side-effect, whereas we've already established that the whole point of this operation is to do side effects.

@gavinking
Copy link
Author

I dunno, "peek" sounds kinda like the opposite of what this is.

I'd be totally happy with calling it "poke", however. 😝

@zeljkot
Copy link

zeljkot commented Jun 1, 2020 via email

@Ladicek
Copy link
Contributor

Ladicek commented Jun 2, 2020

But peek doesn't have any effect -- on the processing pipeline. It lets you see the value and do something with it, but the pipeline isn't affected. Hence, you could also say that affect is the exact opposite 😁

@gavinking
Copy link
Author

Sure, but I'm not writing a code that produces a processing pipeline. I'm writing code that does something interesting to the world. I DGAF what this operation does to the stream, since my program isn't about streams.

@Ladicek
Copy link
Contributor

Ladicek commented Jun 2, 2020

I'm not writing a code that produces a processing pipeline

Yea, well, you do. If you use the Uni API, you build a pipeline, even though there's ever only 1 item flowing through it.

@gavinking
Copy link
Author

Yea, well, you do.

That's the myopic perspective of the designer of a reactive streams library.

It's not my perspective as a user of the API. I'm trying to make an interesting program that does useful things. I actually don't care about reactive streams at all. When I look at my code I want to see information about how my program solves the problem the program is designed to solve. Not information about reactive streams.

@gavinking
Copy link
Author

gavinking commented Jun 2, 2020

Not information about reactive streams.

And that's why

on().onItem().invoke(sideEffect)

while surely very cool and elegant, arguably, to someone who's very myopically interested in reactive streams, is quite intrusive and annoying in my program which does interesting stuff that end users care about.

@Ladicek
Copy link
Contributor

Ladicek commented Jun 2, 2020

Actually it's a perspective of someone who hates reactive programming libraries and believes that if we really need to impose them on users, then we shouldn't try to hide what's really going on, because leaking abstraction etc. etc. etc. But I'm ready to agree to disagree.

@gavinking
Copy link
Author

Well, I'm trying to find a way to hate it slightly less.

@gavinking
Copy link
Author

Alright, so reflecting on @Ladicek point about peek() implying something that has no effect on the stream, I managed to clarify what I dislike about that name, and that suggested a better name.

So why don't I like peek()? Well, "peek" in English means to just sneak a quick glance at the item. Whereas what I'm going to do from this operation is actually use the item to do something very meaningful, perhaps even perform a side-effect on the item itself.

So why not call the operation use()? The name implies to me that I'm going to use the item without affecting or transforming the stream itself. It's a common English word, that's not a Java keyword, and that has just three letters in it!

@gavinking
Copy link
Author

gavinking commented Jun 2, 2020

Hear me out on this one. This reads really naturally, I think:

return factory.withSession(
	session -> session.find(Book.class, bookId)
		.use(book -> session.remove(book))
		.map(book -> "deleted " + book.title)
);

@zeljkot
Copy link

zeljkot commented Jun 2, 2020

Kotlin has also

return factory.withSession(
	session -> session.find(Book.class, bookId)
		.also(book -> session.remove(book))
		.map(book -> "deleted " + book.title)
);

@zeljkot
Copy link

zeljkot commented Jun 2, 2020

I am sure that every designer of the stream library had the same discussion. We already have a standard Java library. While some names may be less than perfect, they are well known and understood, so why not just stick with that? Easy to use reactive library is something to be excited about; a slightly better name for functionality that I already know under a different name is not.

@gavinking
Copy link
Author

I am sure that every designer of the stream library had the same discussion. We already have a standard Java library. While some names may be less than perfect, they are well known and understood, so why not just stick with that?

Ummmmm, because the other APIs are terrible, and terribly confusing?

@cescoffier
Copy link
Contributor

We have tried to use the names from java.util.Stream in MicroProfile reactive streams ops. Feedback has not been good. While "known" it's just confusing and didn't work well (confusing names, hard to find the proper method...).

@cescoffier
Copy link
Contributor

invoke and invokeUni have been added to the Uni class.

@cescoffier cescoffier added this to the 0.6.0 milestone Jul 10, 2020
@cescoffier cescoffier added the enhancement New feature or request label Jul 10, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants