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

make os._wrap_close not inherit from TextIOWrapper #12774

Merged
merged 5 commits into from
Oct 14, 2024
Merged

Conversation

tungol
Copy link
Contributor

@tungol tungol commented Oct 10, 2024

Methods are passed through at runtime via __getattr__

related to #3968

I think is more correct, but we may be getting into territory where duplication of effort becomes a question. Is getting the inheritance right worth maintaining the extra lines of code in this case?

I could see no reason why TextIOWrapper was imported as _TextIOWrapper, other than that import is extremely old; I didn't check but I suspect the current rules around as imports were not as established at the time, and the underscore was added to prevent it appearing in os's namespace. I normalized to just a normal import for that reason.

Methods are passed through at runtime via __getattr__

related to python#3968
@tungol
Copy link
Contributor Author

tungol commented Oct 10, 2024

We could also consider leaving some of the methods out of the stubs since the full scope of what's available on _wrap_close is basically undocumented. reconfigure in particular seems unlikely to be useful, and the same goes for seeking related methods. Actual documentation just says:

The return value is an open file object connected to the pipe, which can be read or written depending on whether mode is 'r' (default) or 'w'. [...] The returned file object reads or writes text strings rather than bytes.

@srittau srittau added the topic: io I/O related issues label Oct 10, 2024

This comment has been minimized.

1 similar comment

This comment has been minimized.

@tungol
Copy link
Contributor Author

tungol commented Oct 11, 2024

I've kept thinking about this one, and the more I think about it the more it seems like picking only a subset of TextIOWrapper's methods to put in the stubs seems like a reasonable idea. If anyone else has thoughts on that, I'd love to hear them.

@tungol tungol mentioned this pull request Oct 11, 2024
@tungol
Copy link
Contributor Author

tungol commented Oct 11, 2024

mypy-primer revealed only read(): #12782 (comment)

I'm not too surprised - the primary use of os.popen probably happens in smaller scripts than what make it into mypy-primer's corpus.

Copy link
Collaborator

@srittau srittau left a comment

Choose a reason for hiding this comment

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

I'm fine with proxying all methods, but if you want to go for a smaller subset (at least until someone complains), I'm fine with that, too.

@tungol
Copy link
Contributor Author

tungol commented Oct 14, 2024

I looked through the first five pages of github code search results for os.popen and saw examples of read, readline, and readlines being used, as well as iteration, but nothing else.

Let's go with the smaller subset to start and see if anyone notices. I updated to include the read* and write* methods, but leave out the rest.

This comment has been minimized.

Copy link
Contributor

According to mypy_primer, this change has no effect on the checked open source code. 🤖🎉

@srittau srittau merged commit 16bab54 into python:main Oct 14, 2024
63 checks passed
@tungol tungol deleted the os branch October 14, 2024 17:06
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
topic: io I/O related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants