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

Mavericks Extensions argsOrNull #639

Merged
merged 3 commits into from
Jul 22, 2022
Merged

Mavericks Extensions argsOrNull #639

merged 3 commits into from
Jul 22, 2022

Conversation

sanogueralorenzo
Copy link
Contributor

Adding Mavericks Extensions argsOrNull which can be useful for those cases where you have an optional argument that might be sent or not.

As an example, imagine a shared screen / flow from different entry points. While this can be fixed in multiple ways in the client's code having an explicit enum that represents null / empty / none it ends up being more code which ultimately represents the same thing (although this introduces the option of nullable arguments which can also be seen as a downside)

Thoughts?

@codecov
Copy link

codecov bot commented Jul 7, 2022

Codecov Report

Merging #639 (a9178c1) into main (f44e85b) will decrease coverage by 5.62%.
The diff coverage is 11.01%.

❗ Current head a9178c1 differs from pull request most recent head cde1959. Consider uploading reports for the commit cde1959 to get more accurate results

@@            Coverage Diff             @@
##             main     #639      +/-   ##
==========================================
- Coverage   55.06%   49.43%   -5.63%     
==========================================
  Files          53       54       +1     
  Lines        2508     2846     +338     
  Branches      326      338      +12     
==========================================
+ Hits         1381     1407      +26     
- Misses        976     1284     +308     
- Partials      151      155       +4     
Impacted Files Coverage Δ
.../airbnb/mvrx/compose/MavericksComposeExtensions.kt 0.00% <ø> (ø)
.../airbnb/mvrx/hilt/HiltMavericksViewModelFactory.kt 18.18% <0.00%> (ø)
...main/kotlin/com/airbnb/mvrx/mocking/MockBuilder.kt 22.01% <0.00%> (-11.73%) ⬇️
...main/kotlin/com/airbnb/mvrx/MavericksExtensions.kt 11.76% <50.00%> (+3.58%) ⬆️
...tlin/com/airbnb/mvrx/MavericksViewModelProvider.kt 88.88% <80.00%> (+0.17%) ⬆️
...tlin/com/airbnb/mvrx/mocking/KotlinReflectUtils.kt 66.66% <81.25%> (+8.04%) ⬆️
...otlin/com/airbnb/mvrx/MavericksMutabilityHelper.kt 78.94% <100.00%> (+1.16%) ⬆️
.../main/kotlin/com/airbnb/mvrx/MavericksViewModel.kt 60.71% <100.00%> (ø)
...rx/src/main/kotlin/com/airbnb/mvrx/PersistState.kt 79.16% <100.00%> (-1.10%) ⬇️
...com/airbnb/mvrx/MavericksViewModelConfigFactory.kt 73.68% <0.00%> (ø)
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ed755e3...cde1959. Read the comment docs.

Copy link
Collaborator

@gpeal gpeal left a comment

Choose a reason for hiding this comment

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

@elihart This seems like a reasonable addition to me. Wdyt?

* creating a key for each one.
*
* To create nullable arguments, define a property in your fragment like:
* `private val listingId by argsOrNull<MyArgs?>()`
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
* `private val listingId by argsOrNull<MyArgs?>()`
* `private val listingId: MyArgs? by argsOrNull()`

Can you update the doc above, too?

*
* Each fragment can only have a single argument with the key [Mavericks.KEY_ARG]
*/
fun <V : Any> argsOrNull() = object : ReadOnlyProperty<Fragment, V?> {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
fun <V : Any> argsOrNull() = object : ReadOnlyProperty<Fragment, V?> {
fun <V> argsOrNull() = object : ReadOnlyProperty<Fragment, V?> {

I think you can safely emit the type narrowing since the return type is nullable.

@sanogueralorenzo
Copy link
Contributor Author

@elihart what do you think of by argsOrNull?

@elihart
Copy link
Contributor

elihart commented Jul 20, 2022

sure, seems helpful. only comment is that it would be nice to have a test

@sanogueralorenzo
Copy link
Contributor Author

Sounds good @elihart added tests cde1959

@sanogueralorenzo sanogueralorenzo requested a review from gpeal July 22, 2022 20:53
Copy link
Contributor

@elihart elihart left a comment

Choose a reason for hiding this comment

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

Thanks!

@elihart elihart merged commit baaf010 into airbnb:main Jul 22, 2022
@sanogueralorenzo sanogueralorenzo deleted the msanoguera/mavericks_extensions_argsornull branch July 23, 2022 07:03
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.

3 participants