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

Update rx-coroutines interop lint message #315

Merged
merged 1 commit into from
Oct 2, 2024

Conversation

serge-slack
Copy link
Contributor

@serge-slack serge-slack commented Oct 2, 2024

Summary

Update rx-coroutines interop lint message to:

rxSingle defaults to Dispatchers.Default. Provide an explicit dispatcher which can be replaced with a test dispatcher to make your tests more deterministic.

As Dispatchers.Unconfined may lead to unintended threading and should only be used deliberately, remove it as a recommendation from the lint message.

Requirements (place an x in each [ ])

The following point can be removed after setting up CI (such as Travis) with coverage reports (such as Codecov)

  • I've written tests to cover the new code and functionality included in this PR.

The following point can be removed after setting up a CLA reporting tool such as cla-assistant.io

@ZacSweers
Copy link
Collaborator

Can we meet in the middle? I think the context is helpful

rxCompletable defaults to Dispatchers.Default, which will silently introduce multithreading. Provide an explicit dispatcher.

@serge-slack
Copy link
Contributor Author

@ZacSweers, how about:

rxCompletable defaults to Dispatchers.Default. Provide an explicit dispatcher.

While it's true that providing no dispatcher can silently can introduce multithreading, imo it also makes the default dispatcher sound less desirable even when used explicitly.

@ZacSweers
Copy link
Collaborator

Let's provide some degree of context why, lint checks should educate as much as they can while guardrailing :)

@serge-slack
Copy link
Contributor Author

Let's provide some degree of context why, lint checks should educate as much as they can while guardrailing :)

In that case I would rather mention that providing an explicit dispatcher aids in testing.

@ZacSweers
Copy link
Collaborator

Sure 👍

@serge-slack serge-slack added this pull request to the merge queue Oct 2, 2024
Merged via the queue into main with commit 852b0c6 Oct 2, 2024
3 checks passed
@serge-slack serge-slack deleted the serge/update-rx-interop-lint branch October 2, 2024 18:54
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.

2 participants