reactor / reactor-kotlin-extensions

Reactor Kotlin Support
Apache License 2.0
143 stars 21 forks source link

Better extensions for sum/average, deprecate others #29

Closed kiwisincebirth closed 3 years ago

kiwisincebirth commented 3 years ago

fix #28

Added missing Kotlin wrappers for all sum() and average() methods provided in reactor.math.MathFlux to the library

simonbasle commented 3 years ago

Sounds like a plan! collectX, xOf and deprecation

kiwisincebirth commented 3 years ago

Sounds like a plan! collectX, xOf and deprecation

Ah, sumAll might be a good one too

Can you clarify your preference, based on comments from above ?

(a) collectX() and xOf() (b) xAll() and xOf() (c) xAll() - Noting there is no method signature conflict between mapped and unmapped functions, so names can be identical. we can just have sumAll() and averageAll() (d) something else

Option (b or c) would be my preference.

PS. Have committed the new methods with deprecations, except naming is not correct at this time. I was more exhaustive in the switch case statements, to account for the most common data types to avoid runtime errors.

simonbasle commented 3 years ago

(a) collectX() and xOf() (b) xAll() and xOf() (c) xAll() - Noting there is no method signature conflict between mapped and unmapped functions, so names can be identical. we can just have sumAll() and averageAll() (d) something else

hehe, so much choice... actually, you have a good point with (c). sumAll() is merely a simplified case of sumAll(Function<T, R:Number>) where T is already a Number sub-class.

to summarize:

Do we still need sumLong...sumBigDecimal and all ?

kiwisincebirth commented 3 years ago

Have made the changes to the code, specifically to name methods as sumAll() and averageAll()

Do we still need sumLong...sumBigDecimal and all ?

I believe so - consider the following code

    val fluxOfInts: Flux<Int> = Flux.just(1,2,3,4,5)
    val sumLong = fluxOfInts.sumLong()
    val avgDouble = fluxOfInts.averageDouble()

if you remove the typed functions the above code would have to be written as

    val fluxOfInts: Flux<Int> = Flux.just(1,2,3,4,5)
    val sumLong = fluxOfInts.sumAll(kotlin.Int::toLong)
    val avgDouble = fluxOfInts.averageAll(kotlin.Int::toDouble)

Which I think is good reason to keep the typed functions.

simonbasle commented 3 years ago

Which I think is good reason to keep the typed functions.

One last naming nitpick/proposal then: what do you think about sumAsXxx to better convey that a conversion is potentially in play? (sumAsLong, sumAsDouble, etc...)

simonbasle commented 3 years ago

oh and while I agree the sumType() methods have value, I really believe the sumType(Function) and averageType(Function) ones should be removed (since sumAll(Function)/averageAll(Function) would be expressive and concise enough to cover these cases).

kiwisincebirth commented 3 years ago

First sorry for not resolving the tests before prior commit.

One last naming nitpick/proposal then: what do you think about sumAsXxx to better convey that a conversion is potentially in play? (sumAsLong, sumAsDouble, etc...)

Yes I like that have changed to include 'As' in all typed methods i.e. sumAsType(), averageAsType()

oh and while I agree the sumType() methods have value, I really believe the sumType(Function) and averageType(Function) ones should be removed (since sumAll(Function)/averageAll(Function) would be expressive and concise enough to cover these cases).

Yep you are right on that, have removed the typed methods with function parameters, that I introduced, cannot remove sumDouble(Function) as it is part of the library

missing @Deprecated annotation. I'm not entirely sure of the syntax and the annotation might need to be imported, as I'm suggesting this from Github's UI.

Added these annotations to the deprecated methods.

Have been through all the comments from above and marked resolved everything where I fixed. See response to your assumption above.

Will commit shortly.

kiwisincebirth commented 3 years ago

resolved the 2 issues 🤞

kiwisincebirth commented 3 years ago

pleasure.