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

[io/wait] Add RBS for "io/wait" #756

Merged
merged 1 commit into from Aug 21, 2021
Merged

Conversation

HoneyryderChuck
Copy link
Contributor

Picking up the work left in #563.

The most controversial part is the mismatch between what documentation
says about return types and what actually gets returned (doc says bool,
but IO or nil are actually returned).

@soutaro
Copy link
Member

soutaro commented Aug 20, 2021

doc says bool, but IO or nil are actually returned

Seems like RDoc is out of date. JA version of the doc says it returns self | bool | nil.

@soutaro
Copy link
Member

soutaro commented Aug 20, 2021

I'm not sure if we can implement a stable test case for the methods...

@HoneyryderChuck
Copy link
Contributor Author

I "bent" the rules a bit to make the tests pass in all rubies. Seems that the behaviour changed 🤷 .

Would it be possible to "pass the ball" to ruby core to fix the return value of these methods, and then rely on rbs test cases to keep it from drifting again? I'm not sure if we can do better here.

Picking up the work left in ruby#563.

The most controversial part is the mismatch between what documentation
says about return types and what actually gets returned (doc says bool,
but IO or nil are actually returned).
Copy link
Member

@soutaro soutaro left a comment

Choose a reason for hiding this comment

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

Will take over this. Thanks! 👍

@soutaro soutaro merged commit 0fa9e57 into ruby:master Aug 21, 2021
@HoneyryderChuck HoneyryderChuck deleted the io-wait branch August 21, 2021 10:03
This was referenced Aug 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants