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

bpo-9372: Deprecate several __getitem__ methods #8609

Merged
merged 8 commits into from Aug 11, 2018

Conversation

Projects
None yet
4 participants
@berkerpeksag
Copy link
Member

commented Aug 1, 2018

The __getitem__ methods of DOMEventStream, FileInput
and FileWrapper classes ignore their 'index'
parameters and return the next item instead.

https://bugs.python.org/issue9372

berkerpeksag added some commits Aug 1, 2018

bpo-9372: Deprecate several __getitem__ methods
The __getitem__ methods of DOMEventStream, FileInput
and FileWrapper classes ignore their 'index'
parameters and return the next item instead.
@serhiy-storchaka
Copy link
Member

left a comment

Please add an entry in What's New.

@@ -169,6 +169,9 @@ available for subclassing as well:
.. deprecated-removed:: 3.6 3.8
The *bufsize* parameter.

.. deprecated:: 3.8
Support for :meth:`sequence protocol <__getitem__>` is deprecated.

This comment has been minimized.

Copy link
@serhiy-storchaka

serhiy-storchaka Aug 1, 2018

Member

Actually the sequence protocol never was supported. It requires __len__ and __reverse__, and better support of __getitem__.

This comment has been minimized.

Copy link
@berkerpeksag

berkerpeksag Aug 1, 2018

Author Member

You're right. Do you have a suggestion for wording? Would something like "Support for __getitem__ method is deprecated." be suitable?

This comment has been minimized.

Copy link
@serhiy-storchaka
r'Use iterator protocol instead'):
with FileInput(files=[t]) as fi:
with self.assertRaises(RuntimeError):
fi[1]

This comment has been minimized.

Copy link
@serhiy-storchaka

serhiy-storchaka Aug 1, 2018

Member

I suggest to use fi[0] which works now and returns "line1\n".

This comment has been minimized.

Copy link
@berkerpeksag

berkerpeksag Aug 1, 2018

Author Member

I used fi[1] on purpose to show that __getitem__ doesn't work as expected (e.g. it would return 'line2\n' if it does)

This comment has been minimized.

Copy link
@serhiy-storchaka

serhiy-storchaka Aug 1, 2018

Member

This is tested in other test (test__getitem__invalid_key). We need to test that even using a "valid" key is deprecated.

@@ -138,6 +139,22 @@ def _ignore_deprecated_imports(ignore=True):
yield


def ignore_warnings(*, category):

This comment has been minimized.

Copy link
@serhiy-storchaka

serhiy-storchaka Aug 1, 2018

Member

Why it is a keyword-only parameter? For a single argument I expect a positional parameter.

This comment has been minimized.

Copy link
@berkerpeksag

berkerpeksag Aug 1, 2018

Author Member

There are many warning-related APIs both in the stdlib and test.support and it's easy to forget how to use them, so I just tried to make the API more strict, easy to remember, and avoid misusing of the function.

@@ -259,6 +259,13 @@ def __next__(self):
# repeat with next file

def __getitem__(self, i):
import warnings
warnings.warn(
"FileInput's __getitem__ method is deprecated. "

This comment has been minimized.

Copy link
@serhiy-storchaka

serhiy-storchaka Aug 1, 2018

Member

The end user likely uses indexing instead of direct calling of __getitem__. What about "Indexing FileInput is deprecated."?

This comment has been minimized.

Copy link
@berkerpeksag

berkerpeksag Aug 1, 2018

Author Member

How about "Indexing of a FileInput object is deprecated." or "Support for indexing a FileInput object is deprecated."

@vadmium do you have a suggestion on wording of the deprecation message?

berkerpeksag added some commits Aug 1, 2018

@berkerpeksag

This comment has been minimized.

Copy link
Member Author

commented Aug 1, 2018

I've addressed all of your review comments.

@serhiy-storchaka
Copy link
Member

left a comment

LGTM, but @vadmium may suggest better wording.

@berkerpeksag berkerpeksag merged commit 84a13fb into python:master Aug 11, 2018

9 checks passed

VSTS: Linux-PR Linux-PR_20180802.27 succeeded
Details
VSTS: Linux-PR-Coverage Linux-PR-Coverage_20180802.23 succeeded
Details
VSTS: Windows-PR Windows-PR_20180802.27 succeeded
Details
VSTS: docs docs_20180802.27 succeeded
Details
VSTS: macOS-PR macOS-PR_20180802.27 succeeded
Details
bedevere/issue-number Issue number 9372 found
Details
bedevere/news News entry found in Misc/NEWS.d
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@berkerpeksag berkerpeksag deleted the berkerpeksag:9372-getitem-deprecation branch Aug 11, 2018

lisroach added a commit to lisroach/cpython that referenced this pull request Sep 10, 2018

bpo-9372: Deprecate several __getitem__ methods (pythonGH-8609)
The __getitem__ methods of DOMEventStream, FileInput,
and FileWrapper classes ignore their 'index' parameters
and return the next item instead.

yahya-abou-imran added a commit to yahya-abou-imran/cpython that referenced this pull request Nov 2, 2018

bpo-9372: Deprecate several __getitem__ methods (pythonGH-8609)
The __getitem__ methods of DOMEventStream, FileInput,
and FileWrapper classes ignore their 'index' parameters
and return the next item instead.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.