conjure-java icon indicating copy to clipboard operation
conjure-java copied to clipboard

More concise visitor builders

Open iamdanfox opened this issue 4 years ago • 0 comments

What happened?

In the apollo repo, we have a ton of nested visitors for various unions whose variants are also more unions. Quite a few of these are just used to map all the possible variants to some simple states like (is this change just a config change), resulting in many visitors where the inputs are actually all unused.

Here's a notional example

FooChange.Visitor.<Boolean>builder()
                    .apple(_appleStateChange -> true)
                    .dragonfruit(_dragonfruitStateChange -> false)
                    .strawberry(_strawberryStateChange -> false)
                    .throwOnUnknown()
                    .build();

Obviously this gets more crazy as the levels of nested visitors increases e.g. 'strawberry' actually has like 7 variants, which we want to granularly map in each case.

See https://internal-github/f/a/pull/8095

What did you want to happen?

I think we could make this significantly more readable if we codegen'd an extra overload of all these builder methods which is either a zero arg function or just takes the value outright, e.g.

FooChange.Visitor.<Boolean>builder()
                    .apple(true)
                    .dragonfruit(false)
                    .strawberry(false)
                    .throwOnUnknown()
                    .build();

or maybe

FooChange.Visitor.<Boolean>builder()
                    .apple(() -> true)
                    .dragonfruit(() -> false)
                    .strawberry(() -> false)
                    .throwOnUnknown()
                    .build();

These should allow mixing and matching, so we could do

FooChange.Visitor.<Boolean>builder()
                    .apple(false)
                    .dragonfruit(() -> lookupFeatureFlag(runtimeConfig))
                    .strawberry(strawberryStateChange -> {
                       // elaborate logic here
                    })
                    .throwOnUnknown()
                    .build();

Possible downsides

A possible downside of this approach is that people end up invoking functions eagerly instead of lazily (kinda like the Optional.orElse/orElseGet mistake). We could potentially use the @CompileTimeStatic annotation to be super conservative initially, and then have a discussion about relaxing this if we really need to??

iamdanfox avatar Jan 26 '21 13:01 iamdanfox