Nir Lisker wrote:
Solved the configuration issues, so I'm continuing with this.

I submitted for the refactoring part.

As mentioned before, this seems OK long as the refactoring is behavior-neutral and limited in scope.

As for, should it be closed as Not an Issue and a new issue created for the new API propasal, or should it be retrofitted?

What new public API are you envisioning to propose? I would recommend to file a new Enhancement request, but leave the existing bug open (you can link it to the new RFE) until there is agreement on any new API, and until we know it solves the problem specified in the bug report.

-- Kevin

- Nir

On Tue, Feb 20, 2018 at 12:45 PM, Nir Lisker < <>> wrote:

    OK, let's wait with this until I figure out if there's a problem
    with the test suit.

    On Fri, Feb 16, 2018 at 11:46 PM, Kevin Rushforth
    < <>>

        This will take me a bit more time to look at than I have right
        now (and Monday is a US holiday), so one quick comment for now:

        The refactoring must not alter any public API signatures for
        exported packagers, and must be behavior neutral. So if there
        are unit tests that pass without your fix and fail with your
        fix, then you will need to alter your fix, unless you can show
        that the tests are testing an implementation detail that would
        not affect an application that just uses public API.

        -- Kevin

        Nir Lisker wrote:
        Let's start with the refactoring then. Before I submit a bug
        let's check that this plan makes sense. Attached webrev for

        * Main change was to the XxxCondition classes. Instead of
        having 4 combinations of primitives and observables I used
        the unification approach that NumberConditionBuilder took
        with wrapping the value in a constant observable. I also
        replaced these classes with a generic wrappers
        XxxConditionHolders which hold the binding part of the
        XxxCondition class.

        * I attempted to generify XxxConditionBuilders as well. The
        attempt is ConditionBuilder and an example implementation was
        done for boolean only - BooleanConditionBuilder2. I think
        that it doesn't gain much.

        * Added BooleanConstant class in internal API (it was missing
        for some reason).

        * The handling of Numbers in the original class is a bit
        weird in my eyes. The compile time return types are
        DoubleBinding if one or both values are double and
        NumberBinding otherwise [1]. On the other hand, the runtime
        return types takes special care to return a binding class
        based on widening conventions and the docs don't mention
        anything about that. In my change, the runtime type is always
        DoubleBinding (though I kept a placeholder if-else chain) and
        that saves some code. The user can always call floatValue()
        etc. anyway.

        Between backwards compatibility and limitations of generics
        this is the best I could come up with.

        Additional notes:

        * Running the tests from gradle causes some of the When tests
        to fail and I don't know why, it's hard to debug. I wrote my
        own ad-hoc test for one of the failed tests and it passes.

        * I noticed that StringConstant extends StringExpression
        while all the other classes just implement their
        respective ObservableXxxValue. Don't know why, but I can
        align these classes to one of those choices.


        On Thu, Feb 15, 2018 at 5:04 AM, Kevin Rushforth
        <>> wrote:

            Sorry for the delay in responding. I was traveling when
            this was sent and barely able to keep up with urgent
            email / tasks.

            Most of what you suggest below seems good. My only
            concern is whether the "on demand" evaluation will have
            any side effects. Conceptually, it seems like the right
            thing to do.

            The refactoring you propose is probably best done as a
            separate bug fix, so that we don't mix behavioral changes
            with refactoring, unless you think that the refactoring
            is intertwined with the fix.

            If you would like to work on a fix, that would be good.
            It will need to include new unit tests, plus ensuring
            that the existing unit tests pass.


            -- Kevin

            Nir Lisker wrote:


                I was looking at
                <>, which
                prompted me to have a look at When. There are a few
                points I would like to

                does not check
                for null as other condition builders do. This results
                in a deeper NPE
                when StringCondition tries to register a listener to the

                * I would like a (re)evaluation on the above bug
                ticket and thoughts on the
                proposal of "on demand evaluation" using a Supplier
                or a similar method.
                The behavior of the intended implementation would be
                to evaluate 'then' and
                'otherwise' whenever their condition is met, and only

                * The class can benefit from some small refactoring,
                such as using
                Objects.requireNonNull for null checks and some code
                reuse to reduce the
                chance of bugs such as the missing null check of

                * There are a few Javadoc corrections and some
                clarifications of the
                current behavior could be beneficial as well.

                I can work on all of the above. How to proceed?

                - Nir

Reply via email to