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

Use kotlin dsl marker for model building receivers #1180

Merged
merged 3 commits into from
May 11, 2021
Merged

Conversation

elihart
Copy link
Contributor

@elihart elihart commented May 8, 2021

I had gotten annoyed by autocomplete suggesting all of our model names inside of building another model - it makes it hard to find the applicable properties of a model as well as enables buggy behavior.

Adding the kotlin dsl marker annotation to both the ModelCollector as well as generated models prevents both of these things.

This is a breaking change if inside of a model building function you reference a property in enclosing EpoxyController - to do so now requires an explicit this. While being safer and more readable that might require some people to make quite a few updates for this.

I was also getting R8 issues that I had to solve by making all modules target java 1.8

* for building models you cannot incorrectly nest models and also don't see cluttered, incorrect
* code completion suggestions.
*/
@DslMarker
Copy link
Collaborator

Choose a reason for hiding this comment

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

TIL

@@ -1,7 +1,7 @@
// Top-level build file where you can add configuration options common to all sub-projects/modules.
buildscript {

ext.KOTLIN_VERSION = "1.4.32"
ext.KOTLIN_VERSION = "1.5.0"
Copy link
Collaborator

Choose a reason for hiding this comment

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

🥳

@elihart elihart merged commit 393a0a6 into master May 11, 2021
@elihart elihart deleted the eli-dsl_marker branch May 11, 2021 19:16
Copy link
Collaborator

@vinaygaba vinaygaba left a comment

Choose a reason for hiding this comment

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

Smart fix to an annoying problem

@elihart elihart mentioned this pull request May 12, 2021
@martymiller
Copy link

@elihart My team recently updated to this version, which required over 500 line changes of heavily verbose code. Seeing that this breaking change was needed because "something annoyed you" seems like a good reason to start migrating away from Epoxy.

@elihart
Copy link
Contributor Author

elihart commented Jul 22, 2022

@martymiller did you see the option to disable this change in #1185?

This wasn't just because it annoyed me, it was trying to fix a problem that I expected would cause friction for many other people. If you see something you would like changed please submit a PR to improve the project; after all, it's free, open source software you're using.

Regarding migrating away, please do move to Compose as it is simply better.

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.

4 participants