Skip to content

gh-115538: Emit warning when use bool as fd in _io.WindowsConsoleIO #116925

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 6 commits into from
Mar 18, 2024
Merged

gh-115538: Emit warning when use bool as fd in _io.WindowsConsoleIO #116925

merged 6 commits into from
Mar 18, 2024

Conversation

aisk
Copy link
Contributor

@aisk aisk commented Mar 17, 2024 •
edited by bedevere-app bot
Loading

@bedevere-app bedevere-app bot mentioned this pull request Mar 17, 2024
Closed
serhiy-storchaka
serhiy-storchaka reviewed Mar 17, 2024
View reviewed changes
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

Could you please add explicit tests for passing boolean to _WindowsConsoleIO constructor?

There are tests that pass 0 and 1 to _WindowsConsoleIO constructor in Lib/test/test_winconsoleio.py. You can add new tests for booleans there.

@aisk
Copy link
Contributor Author

aisk commented Mar 17, 2024 •
edited
Loading

Thank you for the review, test case added. The Tests / CIFuzz (memory) kept failing, but I think it's not caused by this change. Other changes in the main branch code have the same issue.

serhiy-storchaka
serhiy-storchaka reviewed Mar 17, 2024
View reviewed changes
Lib/test/test_winconsoleio.py Outdated
Comment on lines 72 to 73
with warnings.catch_warnings():
warnings.simplefilter("error", RuntimeWarning)
Copy link
Member

Choose a reason for hiding this comment

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

It should not be needed if assertRaises is used.

Copy link
Contributor Author

@aisk aisk Mar 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I tested and found that these two lines are needed. However, I noticed that there is an assertWarns function which does not require these two lines.

The original code is copied from existing codes, so maybe we should update them later.

serhiy-storchaka
serhiy-storchaka approved these changes Mar 18, 2024
View reviewed changes
Copy link
Member

@serhiy-storchaka serhiy-storchaka left a comment

Choose a reason for hiding this comment

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

LGTM.

@serhiy-storchaka serhiy-storchaka enabled auto-merge (squash) March 18, 2024 11:34
@serhiy-storchaka serhiy-storchaka merged commit cd2ed91 into python:main Mar 18, 2024
@aisk aisk deleted the winconsoleio-warning branch March 18, 2024 12:19
vstinner pushed a commit to vstinner/cpython that referenced this pull request Mar 20, 2024
@aisk @vstinner
pythongh-115538: Emit warning when use bool as fd in _io.WindowsConso…
f736c19 
…leIO (pythonGH-116925)
adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 25, 2024
@aisk @adorilson
pythongh-115538: Emit warning when use bool as fd in _io.WindowsConso…
dff2ddb 
…leIO (pythonGH-116925)
diegorusso pushed a commit to diegorusso/cpython that referenced this pull request Apr 17, 2024
@aisk @diegorusso
pythongh-115538: Emit warning when use bool as fd in _io.WindowsConso…
833bc94 
…leIO (pythonGH-116925)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@serhiy-storchaka serhiy-storchaka serhiy-storchaka approved these changes

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
@aisk @serhiy-storchaka