8000
Skip to content

Update rx-coroutines interop lint message#315

Merged
serge-slack merged 1 commit intomainfrom
serge/update-rx-interop-lint
Oct 2, 2024
Merged

Update rx-coroutines interop lint message#315
serge-slack merged 1 commit intomainfrom
serge/update-rx-interop-lint

Conversation

@serge-slack
Copy link
Copy Markdown
Contributor
@serge-slack serge-slack commented Oct 2, 2024 •
edited
Loading

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

@serge-slack serge-slack force-pushed the serge/update-rx-interop-lint branch from 90b7f08 to 24cfb7a Compare October 2, 2024 17:30
@ZacSweers

Copy link
Copy Markdown
Collaborator
ZacSweers commented Oct 2, 2024

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
Copy Markdown
Contributor Author
serge-slack commented Oct 2, 2024

@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
Copy Markdown
Collaborator
ZacSweers commented Oct 2, 2024

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

@serge-slack
Copy link
Copy Markdown
Contributor Author
serge-slack commented Oct 2, 2024

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
Copy Markdown
Collaborator
ZacSweers commented Oct 2, 2024

Sure 👍

@serge-slack serge-slack force-pushed the serge/update-rx-interop-lint branch from 24cfb7a to 64beaef Compare October 2, 2024 18:45
@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
@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

Reviewers

@ZacSweers ZacSweers ZacSweers approved these changes

@MohamedAbdalkader MohamedAbdalkader Awaiting requested review from MohamedAbdalkader

@ryan-moore-slack ryan-moore-slack Awaiting requested review from ryan-moore-slack

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@serge-slack @ZacSweers
0