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

modifier-missing-check: Wrong report for fake content emitters #109

Closed
PaulWoitaschek opened this issue Nov 3, 2022 · 2 comments · Fixed by #110
Closed

modifier-missing-check: Wrong report for fake content emitters #109

PaulWoitaschek opened this issue Nov 3, 2022 · 2 comments · Fixed by #110

Comments

@PaulWoitaschek
Copy link
Contributor

PaulWoitaschek commented Nov 3, 2022

Currently, the modifier-missing-check rule shows an error for things that are technically content emitters but don't emit real content in terms that the content is a different window.

In the following example it does not really make sense to supply a modifier because the composable is really encapsulating it's contents.

@Composable
public fun MyDialog() {
  AlertDialog(
    onDismissRequest = { /*TODO*/ },
    buttons = { Text(text = "Button") },
    text = { Text(text = "Body") },
  )
}

The same applies for composables like ModalBottomSheetLayout.

One solution could be be to extend the current rule logic and check if the function has a single direct content emitter which is one of these window-style content emitters.

Tested with 0.0.22

@mrmans0n
Copy link
Contributor

mrmans0n commented Nov 3, 2022

Hi! Feel free to submit a PR deleting these kind of emitters from https://github.com/twitter/compose-rules/blob/main/core-common/src/main/kotlin/com/twitter/rules/core/util/Composables.kt (or add another list of these types, and then add an exclusion for those in the rule)

mrmans0n added a commit that referenced this issue Nov 9, 2022
* Ignore composables that emit content in their own window. Fixes #109

* Lower case the test names

Co-authored-by: Nacho Lopez <nacho@nlopez.io>

* Refactor the sequence function to be tailrec.

Co-authored-by: Nacho Lopez <nacho@nlopez.io>
@mrmans0n
Copy link
Contributor

mrmans0n commented Nov 9, 2022

Now that this is in, I'm gonna kickstart the publishing process so the changes will be in maven central in version 0.0.23 in an hour or so.

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 a pull request may close this issue.

2 participants