-
Notifications
You must be signed in to change notification settings - Fork 500
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
Introduce Mavericks core module #635
Conversation
Move everything that could be Android platform agnostic into the core module. Part of airbnb#632
Introduce `ExperimentalMavericksApi` annotation to repository declarations, to denote that repository is experimental API. Extract `MavericksBlockExecutions` out from `MavericksViewModelConfig`, as `MavericksViewModelConfig` resides in `mvrx` module. Replace `MavericksRepositoryConfigFactory` with `MavericksRepositoryConfigProvider` for simplicity. Fix docs in `mvrx-core` module
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking really good! mainly just have one question
@@ -30,6 +25,16 @@ fun assertMavericksDataClassImmutability( | |||
) { | |||
require(kClass.java.isData) { "Mavericks state must be a data class! - ${kClass.simpleName}" } | |||
|
|||
val disallowedFieldCollectionTypes = listOfNotNull( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
seems like a good solution 👍
Return back `MavericksViewModel.viewModelScope` for backward compatibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
looks great to me, thanks for contributing this improvement!
Codecov Report
@@ Coverage Diff @@
## main #635 +/- ##
==========================================
- Coverage 55.06% 47.81% -7.25%
==========================================
Files 53 49 -4
Lines 2508 2704 +196
Branches 326 289 -37
==========================================
- Hits 1381 1293 -88
- Misses 976 1279 +303
+ Partials 151 132 -19
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thanks!
@elihart fixed lint issue |
Move everything that could be Android platform agnostic into the core module.
Part of #632