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

Fix of a tryo function #1952

Merged
merged 1 commit into from
Jul 13, 2019
Merged

Conversation

lvitaly
Copy link
Contributor

@lvitaly lvitaly commented Aug 28, 2018

A current implementation of a tryo function has a few bugs. Basically, there are two cases when the function throws an exception instead of returning Failure. Here are these cases as a part of tests.

    "return Failure(_, Full(NPE), _) in case of non empty ignore list without NPE" in {
      val ignore: List[Class[_]] = List(classOf[IllegalArgumentException])

      Box.tryo(ignore)(throw new NullPointerException) must beLike {
        case Failure(_, Full(ex), _) => ex.getClass must_== classOf[NullPointerException]
      }
    }

    "not throw NPE in case of nullable ignore list" in {
      val ignore: List[Class[_]] = null

      Box.tryo(ignore)(throw new IllegalArgumentException) must beLike {
        case Failure(_, Full(ex), _) => ex.getClass must_== classOf[IllegalArgumentException]
      }
    }

The first one is about returning Failure with exception instead of throwing exception in case of exception is not in nonempty ignore list.
The second one is about throwing NPE in case of nullable ignore list. Basically, it is true for other parameters as well. For example for onError param. But I left existing condition for checking ignore list for null. Maybe is better to remove this condition at all or add the same checking to onError parameter.

Copy link
Member

@farmdawgnation farmdawgnation left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep, that definitely looks like it was not intended behavior. Thanks for the fix! I'll let this mellow for a day or so to give other contributors an opportunity to weigh in, but I'm good.

@farmdawgnation
Copy link
Member

Sorry for the delay here, I had a kid late last year and am just now getting back into the swing of OSS. I'm going to give this one final look and merge if it looks good.

@farmdawgnation farmdawgnation merged commit 45782a8 into lift:master Jul 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants