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

Add specs for IO#autoclose? and IO#autoclose= #1077

Merged
merged 2 commits into from
Jun 11, 2024

Conversation

herwinw
Copy link
Member

@herwinw herwinw commented Sep 10, 2023

These are the very basic specs, set a value, get a value. The actual behaviour change is not tested, I tried the example of https://ruby-doc.org/3.2.2/IO.html#method-i-autoclose-3D and a couple of variations, and I could not get them to work.

The actual behaviour is still untested.
@herwinw herwinw marked this pull request as ready for review September 10, 2023 14:50
@io.close
-> { @io.autoclose = false }.should raise_error(IOError, /closed stream/)
end
end
Copy link
Member

@andrykonchin andrykonchin Sep 25, 2023

Choose a reason for hiding this comment

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

Looks good, but it makes sense to have separate test cases for the methods #autoclose= and #autoclose?.

There could also be added the following checks:

  • nil value is treated as false in #autoclose=
  • by default #autoclose? returns true (it's actually describing behaviour of a constructor method)

Copy link
Member Author

Choose a reason for hiding this comment

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

Both additional checks have been added

core/io/autoclose_spec.rb Show resolved Hide resolved
@herwinw herwinw marked this pull request as draft September 25, 2023 10:39
@herwinw herwinw marked this pull request as ready for review September 28, 2023 15:57
@seven1m
Copy link
Contributor

seven1m commented Jun 10, 2024

Was there any additional feedback on this? I wonder if we can merge?

@andrykonchin
Copy link
Member

This wasn't addressed

Looks good, but it makes sense to have separate test cases for the methods #autoclose= and #autoclose?.

neither with with changes nor with starting a discussion.

I don't insist on the change but I believe it should be a conscious decision that we are intentionally mixing specs of two methods (#autoclose and #autoclose?) in the same file and in the same describe section.

@andrykonchin andrykonchin merged commit 8363787 into ruby:master Jun 11, 2024
10 checks passed
@herwinw herwinw deleted the io_autoclose branch June 16, 2024 06:33
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.

3 participants