bpo-34187: _WindowsConsoleIO rises correct exception by augustogoulart · Pull Request #9922

bpo-34187: _WindowsConsoleIO rises correct exception by augustogoulart · Pull Request #9922 · python/cpython

_WindowsConsoleIO implementation of fileno should raise a ValueError
instead of io.UnsupportedOperation if the internal handle value is
INVALID_HANDLE_VALUE.

		/[clinic end generated code: output=006fa74ce3b5cfbf input=079adc330ddaabe6]/
		{
		if (self->fd < 0 && self->handle != INVALID_HANDLE_VALUE) {
		if (self->handle == INVALID_HANDLE_VALUE)

Choose a reason for hiding this comment

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

This if needs curly braces.

Choose a reason for hiding this comment

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

Thanks for making this PR!

ISTM that the potential issue in internal_close is separate from the main issue regarding fileno. I recommend removing the internal_close change from this PR, and possibly creating a separate PR for it.

Regarding the fileno issue, please add a test that reproduces the relevant bug.

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

And if you don't make the requested changes, you will be poked with soft cushions!

I have made the requested changes; please review again.

Thanks for making the requested changes!

@taleinat: please review the changes made to this pull request.

Choose a reason for hiding this comment

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

ISTM this change does more than just raise a different exception, since it also makes sure to close the file handle in a certain case. I'd like to see a test for that too.

This will need a NEWS entry; install blurb and run blurb add to create one.

Also a minor inline comment.

		with ConIO('CONOUT$', 'w') as f:
		self.assertEqual(f.write(b''), 0)

		def test_fileno_raises_correct_exception(self):

Choose a reason for hiding this comment

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

I suggest replacing "correct_exception" with something about the file being closed.

This pull request was closed.