Try.onEach

[nkoder]

What I found missing in Try is possibility to do something on value and return it. For example:

Try {
   // ...
}.onEach {
   printLog(it)
}.map {
   // ...
}

instead of

Try {
   // ...
}.map {
   printLog(it)
   it
}.map {
   // ...
}

[jkpl]

What's the expected behaviour for onEach when the given function throws an exception? Example:

Try {
   ...
}.onEach {
  somethingThatThrowsException()
}.map {
  ...
}

[nkoder]

I think it should be wrapped into Failure in similar way as it is in map. This is the way I implemented it. For example:

        val x: Try<String> = success.map {
            throw RuntimeException("here I am")
        }
        x.get() // exception is thrown here

This way we preserve the whole reason for using Try from funKTionale (or at least the reason I use it), which is to pass exceptions further instead of throwing them in-place.

BTW: another name for onEach I saw for this kind of method in other libraries was peek

[MarioAriasC]

I think that onEach is on the Kotlin standard library, but peek doesn't sound bad.

Can you write a test using the exception scenario?

[nkoder]

I think that onEach is on the Kotlin standard library

right :)

Can you write a test using the exception scenario?

Here it is: https://github.com/MarioAriasC/funKTionale/pull/25/commits/0a93a05de34bf8cea2c141326ed99701c7331698#diff-7ec1a9435e4fb1d41474584cbebf75a7R64 I've written tests in similar way as other tests in same file ( https://github.com/nkoder/funKTionale/blob/0a93a05de34bf8cea2c141326ed99701c7331698/funktionale-try/src/test/kotlin/org/funktionale/tries/TryTest.kt#L59 ) so I used the idea of calling fail() inside method. So there is fail() inside onEach { ... } which is throwing an exception and exception is consumed by onEach.

[nkoder]

OK, I've managed to find another couple of minutes and rewrote tests for foreach and onEach in order to: (1) cover more cases, (2) be more explicit without the need for guessing why given test is really testing something.

Now I think it addresses your concerns @MarioAriasC and @jkpl, but... with this approach, it's less aligned with convention I've met in a test file. I don't know what it's a priority for you.

What is your feedback now? Or maybe it's OK now to merge it to master branch and use it in other apps in a very near future?

[nkoder]

@jkpl I have amended my second commit with two major changes:

1. Fixed false positive test for failure.foreach(...) which was asserting exception in a catch block... but failure.foreach(...) is not meant to throw an exception.

2. Introduced AssertJ

Sadly, amended commit removed your comment and my broad answer to it :( I thought it worked as on GitLab, where discussion on changes is preserved even if amended commit no longer exists.

I will summarize here shortly what I wrote there:

1. Thanks for your comment about assertions inside catch block 👍 I totally forgot about this possibility for false positive tests.

2. All 3 suggested options (try-catch expression, funKTionale Try, TestNG exception testing) seemed to be quite good, although I found some disadvantages in each of them. So I've introduced the 4th solution: AssertJ. This assertions lib has quite descriptive assertions and error messages. And I really love it, and use everyday :-) But... I understand that introducing extra dependency (which introduces another documentation to read) just for some testing corner cases is not quite a best option ;-) So feel free to be against it, then I will choose one of your suggested solutions.

[jkpl]

Sadly, amended commit removed your comment and my broad answer to it :( I thought it worked as on GitLab, where discussion on changes is preserved even if amended commit no longer exists.

No worries. The comments are still there, but they're hidden. Click on "Show outdated" to view the comments.

So I've introduced the 4th solution: AssertJ. This assertions lib has quite descriptive assertions and error messages. And I really love it, and use everyday :-) But... I understand that introducing extra dependency (which introduces another documentation to read) just for some testing corner cases is not quite a best option ;-) So feel free to be against it, then I will choose one of your suggested solutions.

AssertJ looks pretty cool! It allows to be more precise on what is being tested compared to the other alternatives. I personally have no problem with the dependency because it only exists in the test build. @MarioAriasC, what do you think?

"try as an expression": it doesn't look straightforward to me. I mean it will work but will it be easy to understand or more like a clever trick?

I haven't used it much in the context of capturing errors (i.e. val result: Exception?), but I've definitely found it to be useful in cases similar to what the Kotlin documentation demonstrates. I'm slightly biased though because I come from a Scala background, where most things are expressions. :)

[nkoder]

Hi @MarioAriasC @jkpl , any news? Any possibility to have this PR merged? :)

[nkoder]

@MarioAriasC changed commit is pushed. Please check, if it's OK for you now. And if it is, please tell me when can I expect it to be available to use officially, so I will use it in my project :-)

[MarioAriasC]

If everything goes well, we could have a 1.0.1 version tonight.

[nkoder]

BTW: if there is something more to do in funKTionale, then maybe I can sometimes help you guys. Especially if it is kind of clean-up/refactoring stuff. Or maybe it would be better to start updating Wiki to match 1.0 with Try etc.?

[MarioAriasC]

Wiki work will be greatly appreciated

[MarioAriasC]

Sorry, I wasn't unable to deploy it. Tomorrow at UK Night time will be

[MarioAriasC]

Done https://github.com/MarioAriasC/funKTionale/releases/tag/1.0.1

[nkoder]

thanks :) I've incorporated it in my project already.

About Wiki: where to communicate with you, guys, to not spam here? Or maybe just create pull requests to funKTionale.wiki repo?

[MarioAriasC]

Sorry for the late answer bro, really busy.

It will be really helpful if you create a page on the wiki for Try

Cheers