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

Change exception raised from fileno() (and others) to io.UnsupportedOperation #2276

Closed
metasyn opened this issue Feb 24, 2017 · 13 comments
Closed
Labels
good first issue easy issue that is friendly to new contributor type: enhancement new feature or API change, should be merged into features branch type: feature-branch new feature or API change, should be merged into features branch

Comments

@metasyn
Copy link

metasyn commented Feb 24, 2017

In capture, we define a method fileno for the DontReadFromInput:
https://github.com/pytest-dev/pytest/blob/master/_pytest/capture.py#L450

However, python docs suggest this shouldn't be implemented:

file.fileno()

Return the integer “file descriptor” that is used by the underlying implementation to request I/O operations from the operating system. This can be useful for other, lower level interfaces that use file descriptors, such as the fcntl module or os.read() and friends.

Note File-like objects which do not have a real file descriptor should not provide this method!

https://docs.python.org/2/library/stdtypes.html

Is there a particular reason it is provided?

@RonnyPfannschmidt
Copy link
Member

@metasyn most likely to get better errors, but at close evaluation it looks a bit like its a bad idea for code thatd does checks - i wonder if there are other users that work that way

i checked io.StringIO.fileno and its implemented, so i believe its a good idea to check what is done, not what is said (and perhaps use a more specific exception

@nicoddemus
Copy link
Member

Indeed:

Python 3.5.2 | packaged by conda-forge | (default, Jul 26 2016, 02:06:09) [MSC v.1900 64 bit (AMD64)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> import io
>>> io.StringIO().fileno()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
io.UnsupportedOperation: fileno
Python 2.7.11 (v2.7.11:6d1b6a68f775, Dec  5 2015, 20:32:19) [MSC v.1500 32 bit (Intel)] on win32
Type "help", "copyright", "credits" or "license" for more information.
>>> import io
>>> io.StringIO().fileno()
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
io.UnsupportedOperation: fileno

(Same for 2.6)

I guess to follow suit we should raise io.UnsupportedOperation as well, instead of the current ValueError?

@nicoddemus nicoddemus added the type: question general question, might be closed after 2 weeks of inactivity label Feb 25, 2017
@RonnyPfannschmidt
Copy link
Member

Yup

@RonnyPfannschmidt
Copy link
Member

For compat we should check/ensure its a valueerror subclass

@nicoddemus
Copy link
Member

According to the docs:

exception io.UnsupportedOperation
An exception inheriting OSError and ValueError that is raised when an unsupported operation is called on a stream.

So we are all set. 👍

@nicoddemus nicoddemus changed the title Should fileno be implemented on file-like objects that don't support file descriptors? Change exception raised from fileno() (and others) to io.UnsupportedOperation Feb 25, 2017
@nicoddemus nicoddemus added good first issue easy issue that is friendly to new contributor type: feature-branch new feature or API change, should be merged into features branch and removed type: question general question, might be closed after 2 weeks of inactivity labels Feb 25, 2017
@shubheksha
Copy link

I'd like to work on this! From what I understood by going through the above discussion is: I need to change the type of exception in fileno method from ValueError to io.UnsupportedOperation. Is that correct?

@RonnyPfannschmidt
Copy link
Member

yup, in case of trouble on python2.6 we will assist

@metasyn
Copy link
Author

metasyn commented Mar 13, 2017

Sorry slow response here - thanks for your thoughts ya'll. Sounds like a simple change.
@shubheksha are you still interested in doing this? If not, I'd be happy to make a PR.

@RonnyPfannschmidt
Copy link
Member

there is no PR so far, so just make one ^^

@metasyn
Copy link
Author

metasyn commented Mar 14, 2017

@nicoddemus I summited an initial PR for this but realized you suggested in the title rename "(and others)" - could you let me know what you meant (or what I'm missing?)

Thanks!

@nicoddemus
Copy link
Member

Hey thanks for the PR!

I was thinking about the other methods in that class which raise exceptions, specially read and buffer (but we must first see if those methods also raise the same exception).

@nicoddemus
Copy link
Member

A quick look seems like the current behavior of buffer is correct:

>>> from io import StringIO
>>> StringIO().buffer
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
AttributeError: '_io.StringIO' object has no attribute 'buffer'

Not sure if we should change read() though.

nicoddemus pushed a commit to nicoddemus/pytest that referenced this issue Mar 14, 2017
nicoddemus added a commit to nicoddemus/pytest that referenced this issue Mar 14, 2017
@nicoddemus
Copy link
Member

Resolved in #2313, thanks again @metasyn! 👍

This was referenced Mar 6, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue easy issue that is friendly to new contributor type: enhancement new feature or API change, should be merged into features branch type: feature-branch new feature or API change, should be merged into features branch
Projects
None yet
Development

No branches or pull requests

4 participants