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

More user friendly Configuration API #8

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

williamboxhall
Copy link
Contributor

@williamboxhall williamboxhall commented Apr 20, 2020

This is a WIP and does a few things and is best reviewed commit by commit

  • Rename some of the different Configuration.from methods to be more specific to avoid the method overloading which makes compiler messages more convoluted.
  • Extract Configuration out into it's own class since it's getting complicated
  • Introduce a DSL/Builder which makes the compiler errors a little more managable but still slightly awkward.

Calling the builder looks like this:

Configuration.Builder
   .create(PizzaAggregate.Companion::create)
   .update(PizzaAggregate::update)
   .created(PizzaAggregate.Companion::created)
   .updated(PizzaAggregate::update)
   .build()

and changes an error that looks like this:

Error:(28, 27) Kotlin: Type inference failed: Cannot infer type parameter UpdateEvt in inline fun <reified CreationCmd : CreationCommand, CreationEvt : CreationEvent, CmdErr : CommandError, reified UpdateCmd : UpdateCommand, UpdateEvt : UpdateEvent, reified Agg : Aggregate> from(noinline create: (CreationCmd) -> Either<CmdErr, CreationEvt>, noinline update: Agg.(UpdateCmd) -> Either<CmdErr, List<UpdateEvt>>, noinline created: (CreationEvt) -> Agg, noinline updated: Agg.(UpdateEvt) -> Agg = ..., noinline aggregateType: Agg.() -> String = ...): Configuration<CreationCmd, CreationEvt, CmdErr, UpdateCmd, UpdateEvt, Agg>
None of the following substitutions
((PizzaCreationCommand) -> Either<PizzaError, PizzaCreated>,Aggregate.(PizzaUpdateCommand) -> Either<PizzaError, List<PizzaUpdateEvent>>,(PizzaCreated) -> Aggregate,Aggregate.(PizzaUpdateEvent) -> Aggregate,Aggregate.() -> String)
((PizzaCreationCommand) -> Either<PizzaError, PizzaCreated>,Aggregate.(PizzaUpdateCommand) -> Either<PizzaError, List<UpdateEvent>>,(PizzaCreated) -> Aggregate,Aggregate.(UpdateEvent) -> Aggregate,Aggregate.() -> String)
((PizzaCreationCommand) -> Either<PizzaError, PizzaCreated>,Aggregate.(PizzaUpdateCommand) -> Either<PizzaError, List<@ParameterName PizzaUpdateCommand>>,(PizzaCreated) -> Aggregate,Aggregate.(PizzaUpdateCommand) -> Aggregate,Aggregate.() -> String)
can be applied to
(KFunction1<@ParameterName PizzaCreationCommand, Either<PizzaError, PizzaCreated>>,KFunction2<PizzaAggregate, @ParameterName PizzaUpdateCommand, Either<PizzaError, List<PizzaUpdateEvent>>>,KFunction1<@ParameterName PizzaCreationEvent, PizzaAggregate>,KFunction2<PizzaAggregate, @ParameterName PizzaUpdateCommand, Either<PizzaError, List<PizzaUpdateEvent>>>)

into

Error:(26, 30) Kotlin: Type mismatch: inferred type is KFunction2<PizzaAggregate, @ParameterName PizzaUpdateCommand, Either<PizzaError, List<PizzaUpdateEvent>>> but PizzaAggregate.(PizzaUpdateEvent) -> PizzaAggregate was expected

The compiler errors from Configuration.from are confusing enough when
working with the 4 different function handles without the additional
confounder of there being four different versions of the from method
which take different arguments. This should help cut down on the
confusion.
@williamboxhall williamboxhall changed the title More user friendly Configuration API WIP More user friendly Configuration API Apr 20, 2020
Changes an error that looks like this:
```
Error:(28, 27) Kotlin: Type inference failed: Cannot infer type parameter UpdateEvt in inline fun <reified CreationCmd : CreationCommand, CreationEvt : CreationEvent, CmdErr : CommandError, reified UpdateCmd : UpdateCommand, UpdateEvt : UpdateEvent, reified Agg : Aggregate> from(noinline create: (CreationCmd) -> Either<CmdErr, CreationEvt>, noinline update: Agg.(UpdateCmd) -> Either<CmdErr, List<UpdateEvt>>, noinline created: (CreationEvt) -> Agg, noinline updated: Agg.(UpdateEvt) -> Agg = ..., noinline aggregateType: Agg.() -> String = ...): Configuration<CreationCmd, CreationEvt, CmdErr, UpdateCmd, UpdateEvt, Agg>
None of the following substitutions
((PizzaCreationCommand) -> Either<PizzaError, PizzaCreated>,Aggregate.(PizzaUpdateCommand) -> Either<PizzaError, List<PizzaUpdateEvent>>,(PizzaCreated) -> Aggregate,Aggregate.(PizzaUpdateEvent) -> Aggregate,Aggregate.() -> String)
((PizzaCreationCommand) -> Either<PizzaError, PizzaCreated>,Aggregate.(PizzaUpdateCommand) -> Either<PizzaError, List<UpdateEvent>>,(PizzaCreated) -> Aggregate,Aggregate.(UpdateEvent) -> Aggregate,Aggregate.() -> String)
((PizzaCreationCommand) -> Either<PizzaError, PizzaCreated>,Aggregate.(PizzaUpdateCommand) -> Either<PizzaError, List<@ParameterName PizzaUpdateCommand>>,(PizzaCreated) -> Aggregate,Aggregate.(PizzaUpdateCommand) -> Aggregate,Aggregate.() -> String)
can be applied to
(KFunction1<@ParameterName PizzaCreationCommand, Either<PizzaError, PizzaCreated>>,KFunction2<PizzaAggregate, @ParameterName PizzaUpdateCommand, Either<PizzaError, List<PizzaUpdateEvent>>>,KFunction1<@ParameterName PizzaCreationEvent, PizzaAggregate>,KFunction2<PizzaAggregate, @ParameterName PizzaUpdateCommand, Either<PizzaError, List<PizzaUpdateEvent>>>)
```
into

```
Error:(26, 30) Kotlin: Type mismatch: inferred type is KFunction2<PizzaAggregate, @ParameterName PizzaUpdateCommand, Either<PizzaError, List<PizzaUpdateEvent>>> but PizzaAggregate.(PizzaUpdateEvent) -> PizzaAggregate was expected
```
Frustratingly these have to be public for reified types to work, and
since the reified types are super valuable I'm just doing this to make
it obvious to the caller not to use them.
@williamboxhall williamboxhall changed the title WIP More user friendly Configuration API More user friendly Configuration API May 19, 2020
@williamboxhall williamboxhall marked this pull request as ready for review May 19, 2020 06:42
@admackin
Copy link
Contributor

I'm wondering whether we should abandon the configuration building options, and just insist on using TypedAggregate etc.. The 'configuration args' approach that you're improving here is probably more friendly for ruby devs, as you note, but this is not Ruby and as you've discovered, setting this up to work and produce sensible compiler errors is very challenging. But, if you just set up the generic type params once-off in your aggregate class implementation, everything Just Works, and then the configuration is just a one-liner, and the compiler errors are already sensible. This is also in line with being an opinionated framework, which I think you're in favour of @williamboxhall. I think that part of being opinionated is making sure there is one, and only one (nice clean simple) way to do things

(aside: coming from a Python background here – I get very frustrated by things such as method aliases in Ruby! Now we have to choose one of several functionally identical ways to do a standard thing, and it may not look like what other devs chose in the same codebase. And now we have to run Rubocop to tell us to use one over the other!).

In Kotlin, i think the TypedAggregate approach is closer to being the one way we should pick, if we have to pick one: it is fairly idiomatic Kotlin code, it is simple for Kotlin devs to understand and implement their approach. The difference in setup from what we do in Ruby is unfortunate, but we should lean in to Kotlin IMO instead of hacking it to be a bit more Ruby-ish.

The other way to be opinionated would be avoid the TypedAggregate approach entirely and lean into the approach you've made improvements on here. But, you've had to jump through some hoops on the implementation side to get us there (and we have to remember/infer from type hints which order to call .create(d)/.update(d) on the builder).

Another aside: I spent a bit of time this morning trying to write a nice fleshed out DSL (just to see if I could) based on your work here, so you could do something like:

        configuration {
            create(PizzaAggregate.Companion::create)
            update(PizzaAggregate::update)
            created(PizzaAggregate.Companion::created)
            updated(PizzaAggregate::updated)
        },

but I couldn't get the type inference to work unfortunately, so it ended up being just as verbose as the TypedAggregate option

Copy link
Contributor

@admackin admackin left a comment

Choose a reason for hiding this comment

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

Looks pretty reasonable. Do you still need a detailed review on this @williamboxhall ?

@williamboxhall
Copy link
Contributor Author

Ah thanks for the look @admackin! This has definitely gotten stale so I reckon hold off on a detailed review for now and I might rebase this and go over it again when I have a chance.

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