eclipse / microprofile-reactive-streams-operators

Microprofile project
Apache License 2.0
80 stars 33 forks source link

PublisherBuilder and ProcessorBuilder doesn't document the requirement of immutability #142

Closed akarnokd closed 4 years ago

akarnokd commented 4 years ago

Currently, neither PublishBuilder or ProcessorBuilder documents that it has to be immutable explicitly, it only turns out to be a requirement due to the TCK test builderShouldBeImmutable.

First of all, Builders in general are thought to be mutable (see StringBuilder) and users have to fork off via some build method to fixate the setup while continuing to build more.

Second, by requiring immutability as well as hosting a Graph, chaining up operators results in a lot of copying. I'd think it is very uncommon to fork off builders in general and most usages is a linear sequence of operator applications followed by one build call.

Azquelt commented 4 years ago

The methods on PublisherBuilder do all say that they return a new PublisherBuilder, rather than modifying and returning the same PublisherBuilder, but I agree we should also say this explicitly in the class doc.

To your first point, we can't easily make PublisherBuilder mutable because it is parameterized with the type that it produces and many of its methods (e.g. .map())return a PublisherBuilder which produces a different type. While it would be possible to have the PublisherBuilder unsafely cast and return itself, this would be fairly confusing for the user since they would receive an object of a different type but it's actually the same object.

To your second point I can see that it does force you to create at least three objects per operator (the PublisherBuilder, the Stage and finally whatever object actually implements the reactive operation). Changing this would require a big rethink of the whole design which isn't feasible at the moment.