Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Refactor streams: rename is_* to wait_* for clarity #2069

Merged
merged 1 commit into from
Feb 20, 2025

Conversation

falbrechtskirchinger
Copy link
Contributor

  • Replace is_readable() with wait_readable() and is_writable() with wait_writable() in the Stream interface.
  • Implement a new is_readable() function with semantics that more closely reflect its name. It returns immediately whether data is available for reading, without waiting.
  • Update call sites of is_writable(), removing redundant checks.

Note: The new is_readable() capability is essential for the WebSockets code, as there's currently no way to tell if read() would block. I have an alternative proposal to add has_buffered_data(), which is identical to is_readable() in this PR. This alternative is less invasive and requires no other code changes, while this PR also aims to enhance semantic clarity.

- Replace is_readable() with wait_readable() and is_writable() with
  wait_writable() in the Stream interface.
- Implement a new is_readable() function with semantics that more
  closely reflect its name. It returns immediately whether data is
  available for reading, without waiting.
- Update call sites of is_writable(), removing redundant checks.
@falbrechtskirchinger falbrechtskirchinger changed the title Refactor streams: rename is_\* to wait_\* for clarity Refactor streams: rename is_* to wait_* for clarity Feb 19, 2025
@yhirose
Copy link
Owner

yhirose commented Feb 20, 2025

@falbrechtskirchinger thanks for the improvement! is_readable() is no longer used, right? If so, could you please remove it?

@falbrechtskirchinger
Copy link
Contributor Author

@falbrechtskirchinger thanks for the improvement! is_readable() is no longer used, right? If so, could you please remove it?

Please read the note in the OP. is_readable() is critical for the PRs I'll submit later today. Once all three features are in, I'll add unit tests.

@yhirose
Copy link
Owner

yhirose commented Feb 20, 2025

Sorry that I forgot to pay attention to the note... The code looks good. Thanks!

@yhirose yhirose merged commit 550f728 into yhirose:master Feb 20, 2025
7 of 9 checks passed
@falbrechtskirchinger
Copy link
Contributor Author

Sorry that I forgot to pay attention to the note... The code looks good. Thanks!

No worries! 👍

@falbrechtskirchinger falbrechtskirchinger deleted the stream-api-prop1 branch February 20, 2025 18:07
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants