bpo-5945: mapping.rst: PyMapping_Check(list) returns 1 #144

jbarlow83 · 2017-02-17T21:50:38Z

As discussed below, in Python 3, PyMapping_Check(list) returns 1. This behavior is justified by a developer as consistent with Python 3's internals. It is however surprising to C-API users (speaking from experience) and because it deviates from collections.abc.Mapping.

I think it is better to be vague than describe the implementation details.

https://mail.python.org/pipermail/python-dev/2009-May/089445.html

https://bugs.python.org/issue5945

the-knights-who-say-ni · 2017-02-17T21:50:39Z

Hello, and thanks for your contribution!

I'm a bot set up to make sure that the project can legally accept your contribution by verifying you have signed the PSF contributor agreement (CLA).

Unfortunately we couldn't find an account corresponding to your GitHub username on bugs.python.org (b.p.o) to verify you have signed the CLA. This is necessary for legal reasons before we can look at your contribution. Please follow these steps to help rectify the issue:

If you don't have an account on b.p.o, please create one
Make sure your GitHub username is listed in "Your Details" at b.p.o
If you have not already done so, please sign the PSF contributor agreement
If you just signed the CLA, please wait at least one US business day and then check "Your Details" on bugs.python.org to see if your account has been marked as having signed the CLA (the delay is due to a person having to manually check your signed CLA)
Reply here saying you have completed the above steps

Thanks again to your contribution and we look forward to looking at it!

serhiy-storchaka · 2017-02-18T09:16:05Z

Doc/c-api/mapping.rst

@@ -9,7 +9,8 @@ Mapping Protocol
 .. c:function:: int PyMapping_Check(PyObject *o)

   Return ``1`` if the object provides mapping protocol, and ``0`` otherwise.  This
-   function always succeeds.
+   function always succeeds. This is not exactly equivalent to ``collections.abc.Mapping``.


Use double space after sentence-ending period. The line is too long, wrap it at column 79. Make collections.abc.Mapping a reference.

louisom · 2017-02-24T13:04:47Z

Should the commit rebase into one commit?

jbarlow83 · 2017-02-24T20:01:44Z

@grapherd The PR can be accepted as a squash merge in one click that will have the same effect as manually squashing it.

As discussed below, in Python 3, PyMapping_Check(list) returns 1. This behavior is justified by a developer as consistent with Python 3's internals. It is however surprising to C-API users (speaking from experience) and because it deviates from collections.abc.Mapping. https://mail.python.org/pipermail/python-dev/2009-May/089445.html Squashed a few revisions

jbarlow83 · 2017-02-27T20:09:30Z

Rebased to HEAD and squashed

Mariatta · 2017-03-06T06:10:25Z

I think this looks good now. @serhiy-storchaka would you like to take another look? I can do the backports.
Thanks :)

rhettinger · 2017-03-06T07:12:21Z

No backports please.

serhiy-storchaka · 2017-03-06T07:52:52Z

This is just the documentation fix. I think it should be backported.

But proposed wording doesn't LGTM. See my comment on the tracker.

jbarlow83 · 2017-03-10T21:46:26Z

On bpo serhiy-storchaka wrote:

Proposed wording doesn't looks much informative to me. Maybe just say that PyMapping_Check() also returns 1 for sequences that support slicing? And recommend PyMapping_Check() && !PySequence_Check() for true mapping test?

There is no recommendation to use PyMapping_Check() && !PySequence_Check() in that thread. In fact developers recommend against both PyMapping_Check and PySequence_Check as well:

(Author: Benjamin Peterson) Personally, I think PyMapping_Check and PySequence_Check should be deprecated and removed. Like their Python counterparts, operator.isMappingType() and operation.isSequenceType(), they are unreliable at best in the face of not LBYL and abcs

So I think it is appropriate to keep the proposed wording since it steers users to ABC checks and away from these APIs. If there is a change perhaps it should be explicitly "not recommended" in the documentation.

…iew_new Replace copied code by a limited API function. This fixes the assertion error caused by commit 2eea952.

Stackless contributes two tests to builtins: TaskletExit and TaskletExit.__init__. Therefore we have to adjust the limit. Add missing changelog entries (python#143, python#144).

brettcannon · 2018-02-21T01:53:27Z

To try and help move older pull requests forward, we are going through and backfilling 'awaiting' labels on pull requests that are lacking the label. Based on the current reviews, the best we can tell in an automated fashion is that a core developer requested changes to be made to this pull request.

If/when the requested changes have been made, please leave a comment that says, I have made the requested changes; please review again. That will trigger a bot to flag this pull request as ready for a follow-up review.

jbarlow83 · 2018-02-23T01:11:53Z

I have made the requested changes; please review again.

Given that several core developers suggested or concurred with deprecating this API (Benjamin Peterson, GvR, Raymond Hettinger) I marked it not recommended as an alternative to officially deprecating.

I think it is best to omit the suggestion to usePyMapping_Check() && !PySequence_Check(). It appears this test does not work in certain cases, because PyMapping_Check is actually testing an CPython implementation detail. Instead I recommended PyObject_IsInstance which steers the developer toward the modern ABC API.

bedevere-bot · 2018-02-23T01:11:55Z

Thanks for making the requested changes!

@serhiy-storchaka: please review the changes made to this pull request.

…iew_new Replace copied code by a limited API function. This fixes the assertion error caused by commit 2eea952. (cherry picked from commit 096ad87)

Stackless contributes two tests to builtins: TaskletExit and TaskletExit.__init__. Therefore we have to adjust the limit. Add missing changelog entries (python#143, python#144). (cherry picked from commit 7327e4b)

willingc

@jbarlow83 Thanks for the PR and the changes. To keep this PR from further languishing, I suppose it may be most efficient to update the wording to incorporate @serhiy-storchaka's suggestion.

willingc · 2018-05-21T04:44:33Z

Doc/c-api/mapping.rst

-   Return ``1`` if the object provides mapping protocol, and ``0`` otherwise.  This
-   function always succeeds.
+   **Not recommended.** Return ``1`` if the object provides the C-API mapping
+   protocol, and ``0`` otherwise.  This function always succeeds.  The C-API


I recommend using @serhiy-storchaka's suggested wording to move this PR forward.

~~The C-API mapping protocol is not equivalent to :class:collections.abc.Mapping, and also returns 1 for sequences that support slicing. Use :c:func::PyObject_IsInstance instead.~~

PyMapping_Check() also returns 1 for sequences that support slicing.

jbarlow83 · 2018-05-21T05:01:10Z

@willingc Done

willingc · 2018-05-21T05:02:40Z

Thanks @jbarlow83 for the quick response.

@serhiy-storchaka This looks good to me now. If you have no further changes, I recommend merging. Thanks.

ilevkivskyi · 2018-06-04T10:34:54Z

Superseded by #7029

the-knights-who-say-ni added the CLA not signed label Feb 17, 2017

Mariatta removed the CLA not signed label Feb 18, 2017

the-knights-who-say-ni added the CLA signed label Feb 18, 2017

jbarlow83 changed the title ~~mapping.rst: PyMapping_Check(list) returns 1~~ bpo-5945: mapping.rst: PyMapping_Check(list) returns 1 Feb 18, 2017

serhiy-storchaka requested changes Feb 18, 2017

View reviewed changes

serhiy-storchaka added the docs Documentation in the Doc dir label Feb 18, 2017

berkerpeksag added needs backport to 3.5 labels Feb 19, 2017

jbarlow83 force-pushed the docs-pymapping-check branch from e9ad4cc to 0c7abc1 Compare February 27, 2017 20:08

brettcannon removed the needs backport to 3.5 label Aug 11, 2017

akruis pushed a commit to akruis/cpython that referenced this pull request Dec 20, 2017

Stackless issue python#144: Replace copied code in pricklepit.c dictv…

096ad87

…iew_new Replace copied code by a limited API function. This fixes the assertion error caused by commit 2eea952.

brettcannon added the awaiting changes label Feb 21, 2018

Update mapping.rst

be98854

bedevere-bot added awaiting change review and removed awaiting changes labels Feb 23, 2018

willingc reviewed May 21, 2018

View reviewed changes

Remove alternative to PyMapping_Check

463418b

willingc approved these changes May 21, 2018

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels May 21, 2018

ilevkivskyi closed this Jun 4, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-5945: mapping.rst: PyMapping_Check(list) returns 1 #144

bpo-5945: mapping.rst: PyMapping_Check(list) returns 1 #144

jbarlow83 commented Feb 17, 2017 •

edited by bedevere-bot

Loading

the-knights-who-say-ni commented Feb 17, 2017

serhiy-storchaka Feb 18, 2017

louisom commented Feb 24, 2017

jbarlow83 commented Feb 24, 2017

jbarlow83 commented Feb 27, 2017 •

edited

Loading

Mariatta commented Mar 6, 2017

rhettinger commented Mar 6, 2017

serhiy-storchaka commented Mar 6, 2017

jbarlow83 commented Mar 10, 2017

brettcannon commented Feb 21, 2018

jbarlow83 commented Feb 23, 2018 •

edited

Loading

bedevere-bot commented Feb 23, 2018

willingc left a comment

willingc May 21, 2018

jbarlow83 commented May 21, 2018

willingc commented May 21, 2018

ilevkivskyi commented Jun 4, 2018

bpo-5945: mapping.rst: PyMapping_Check(list) returns 1 #144

bpo-5945: mapping.rst: PyMapping_Check(list) returns 1 #144

Conversation

jbarlow83 commented Feb 17, 2017 • edited by bedevere-bot Loading

the-knights-who-say-ni commented Feb 17, 2017

serhiy-storchaka Feb 18, 2017

Choose a reason for hiding this comment

louisom commented Feb 24, 2017

jbarlow83 commented Feb 24, 2017

jbarlow83 commented Feb 27, 2017 • edited Loading

Mariatta commented Mar 6, 2017

rhettinger commented Mar 6, 2017

serhiy-storchaka commented Mar 6, 2017

jbarlow83 commented Mar 10, 2017

brettcannon commented Feb 21, 2018

jbarlow83 commented Feb 23, 2018 • edited Loading

bedevere-bot commented Feb 23, 2018

willingc left a comment

Choose a reason for hiding this comment

willingc May 21, 2018

Choose a reason for hiding this comment

jbarlow83 commented May 21, 2018

willingc commented May 21, 2018

ilevkivskyi commented Jun 4, 2018

jbarlow83 commented Feb 17, 2017 •

edited by bedevere-bot

Loading

jbarlow83 commented Feb 27, 2017 •

edited

Loading

jbarlow83 commented Feb 23, 2018 •

edited

Loading