-
Notifications
You must be signed in to change notification settings - Fork 20
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
Code generation proposal #60
Comments
What are we gonna do about fun mainReducer(settingsReducer: SettingsReducer): Reducer<AppState, AppAction> =
settingsReducer.optionalPullback { appState -> listOfItems.isNotEmpty() } 🤔 |
I don't hate the naming, I'll just leave some possible alternatives here for consideration: WrapsAction
ChildStates
|
It'd be super cool to have an option to replace some of the generated functions with your own implementation if needed. Would something like this be possible? fun Reducer<SettingsState, SettingsAction>.pullback(
mapToLocalState: (AppState) -> SettingsState = ::mapAppStateToSettingsState,
mapToLocalAction: (AppAction) -> SettingsAction? = ::mapAppActionToSettingsAction,
mapToGlobalState: (AppState, SettingsState) -> AppState = ::mapSettingsStateToAppState,
mapToGlobalAction: (SettingsAction) -> AppAction = ::mapSettingsActionToAppAction,
) = pullback(
mapToLocalState,
mapToLocalAction,
mapToGlobalState,
mapToGlobalAction,
) |
This is probably an edge case so I don't think we need to address this now but what if |
The biggest obstacle in Toggl App would be the fact that we have a ton of custom mappings where we often map types to different types like her for example: fun mapAppStateToCalendarState(appState: AppState): CalendarState =
CalendarState(
user = appState.user,
workspaces = appState.workspaces.getOrEmptyMap(),
organizations = appState.organizations.getOrEmptyMap(),
timeEntries = appState.timeEntries.getOrEmptyMap(),
....
) But I guess that's our problem :-D |
Good point. I think mapping should JustWork™ and be able to figure out optional state based on nullability, but if you want something like having optional state based on a property then yeah, we need to add an extra parameter. I'll cook something
I started with
That was my first idea, but I opted for making it simpler because I didn't see the need for disambiguation. Doesn't hurt to be explicit 🤷
I think the solution of having overridable functions in the generated pullback method could be a way of solving that. We can go as far as making the whole thing augmentable rather than replaceable, e.g.: interface ReducerMapperInterfaceThatSeriouslyNeedABetterName<ParentState, ParentAction, ChildState, ChildAction> {
fun mapToParentAction(childAction: ChildAction): ParentAction
fun mapToParentState(parentState: ParentState, childState: ChildState): ParentState
fun mapToChildAction(parentaction: ParentAction): ChildAction?
fun mapToChildState(parentState: ParentState): ChildState
} The compiler plugin could generate an implementation of this interface with virtual methods that you could override if you want to provide specific behavior without losing the generated bits. I like the solution of just using functions though, so please advise if you think this is needed. |
Proposal
Since the inception of this library I've been meaning to write a compiler plugin which could generate a lot of the boilerplate needed to overcome the lack of Keypaths in Kotlin. I have a Proof of concept plugin but I need to discuss the details to make sure I'm covering all details. Since I don't have access to the Toggl codebase anymore, I will need internal help to find edge cases
My idea is simple:
SymbolProcessor
that will generate the mapping and pullback functions for youinline fun <From, reified To> From.unwrap(): To? =
everywhere so I can finally have peace of mindThis is the first draft I have of the wishful thinking outcome of this proposal. I'd love to hear feedback on naming, but also on possible ways of improving the proposal, edge cases I didn't consider and boilerplate we could automate that I'm not touching here yet.
Tasks
komposable-architecture-compiler
artifactThe text was updated successfully, but these errors were encountered: