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 collectAsStateWithLifecycle #5889

Merged

Conversation

Rawa
Copy link
Contributor

@Rawa Rawa commented Mar 4, 2024

Use collectAsStateWithLifecycle, and ensure all uses state as variable name instead of uiState.


This change is Reviewable

Copy link

linear bot commented Mar 4, 2024

@Rawa Rawa self-assigned this Mar 4, 2024
@Rawa Rawa requested a review from albin-mullvad March 4, 2024 09:41
@Rawa Rawa added the Android Issues related to Android label Mar 4, 2024
Copy link
Collaborator

@albin-mullvad albin-mullvad left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 28 of 28 files at r1, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@Rawa Rawa force-pushed the start-using-collectasstatewithlifecycle-droid-563 branch from a92362e to 5ccd4c9 Compare March 4, 2024 10:57
@albin-mullvad albin-mullvad force-pushed the start-using-collectasstatewithlifecycle-droid-563 branch from 5ccd4c9 to be212ca Compare March 4, 2024 12:32
@Rawa Rawa force-pushed the start-using-collectasstatewithlifecycle-droid-563 branch from be212ca to 6dead93 Compare March 5, 2024 09:16
@Pururun Pururun requested a review from albin-mullvad March 5, 2024 09:18
Copy link
Contributor

@Pururun Pururun left a comment

Choose a reason for hiding this comment

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

Looks good, just a small thought since this PR does two things that is easy to miss in a future feature, do we want to add some konsist check for using collectAsLifecycle and that we want to call the state state and not uiState?

Reviewed 3 of 3 files at r2.
Reviewable status: 24 of 31 files reviewed, all discussions resolved (waiting on @albin-mullvad)

Copy link
Collaborator

@albin-mullvad albin-mullvad left a comment

Choose a reason for hiding this comment

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

Reviewed 3 of 3 files at r2, 7 of 7 files at r3, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

Copy link
Contributor Author

@Rawa Rawa left a comment

Choose a reason for hiding this comment

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

Konsist does not allow for inspecting the variable names, only the signatures of methods and also import statements. So the alternatives would be:

  1. Ensure no composables take an argument that is named uiState
  2. We never import function collectAsState

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@albin-mullvad albin-mullvad force-pushed the start-using-collectasstatewithlifecycle-droid-563 branch from 6dead93 to 12a0137 Compare March 5, 2024 09:28
Copy link
Collaborator

@albin-mullvad albin-mullvad left a comment

Choose a reason for hiding this comment

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

Both of those makes sense imo. Should be quite straight forward to add, right? I would say we go ahead and add those 🚀

Reviewed all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@Rawa Rawa requested review from albin-mullvad and Pururun March 5, 2024 10:09
Copy link
Contributor

@Pururun Pururun left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@albin-mullvad albin-mullvad force-pushed the start-using-collectasstatewithlifecycle-droid-563 branch from 2691360 to ca56675 Compare March 5, 2024 11:43
Copy link
Collaborator

@albin-mullvad albin-mullvad left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@albin-mullvad albin-mullvad merged commit 8e993df into main Mar 5, 2024
30 checks passed
@albin-mullvad albin-mullvad deleted the start-using-collectasstatewithlifecycle-droid-563 branch March 5, 2024 11:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Android Issues related to Android
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants