Skip to content

bpo-39142: Avoid converting namedtuple instances to ConvertingTuple. #17773

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 2 commits into from
Jan 1, 2020
Merged

bpo-39142: Avoid converting namedtuple instances to ConvertingTuple. #17773

merged 2 commits into from
Jan 1, 2020

Conversation

vsajip
Copy link
Member

@vsajip vsajip commented Dec 31, 2019 •
edited by bedevere-bot
Loading

This uses the heuristic of assuming a named tuple is a subclass of
tuple with a _fields attribute. This change means that contents of
a named tuple wouldn't be converted - if a user wants to have
ConvertingTuple functionality from a namedtuple, they will have to
implement it themselves.

https://bugs.python.org/issue39142

@vsajip
bpo-39142: Avoid converting namedtuple instances to ConvertingTuple.
91d74aa 
This uses the heuristic of assuming a named tuple is a subclass of
tuple with a _fields attribute. This change means that contents of
a named tuple wouldn't be converted - if a user wants to have
ConvertingTuple functionality from a namedtuple, they will have to
implement it themselves.
@vsajip vsajip added type-bug An unexpected behavior, bug, or error skip news labels Dec 31, 2019
@vsajip
Copy link
Member Author

vsajip commented Dec 31, 2019

@tirkarthi - I would appreciate any comments you might have on this PR. Thanks for the analysis on the issue.

@vsajip vsajip removed the skip news label Dec 31, 2019
corona10
corona10 reviewed Jan 1, 2020
View reviewed changes
Copy link
Member

@corona10 corona10 left a comment

Choose a reason for hiding this comment

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

Just comment:
We don't have to care structseq type in this issue?

@vsajip
Copy link
Member Author

vsajip commented Jan 1, 2020 •
edited by bedevere-bot
Loading

We don't have to care structseq type in this issue?

I would say not, because it's easy and common to create a namedtuple in Python but structseq is generally used by C API code to create analogous objects, so occurrences are fewer. If a use case comes up that requires structseq support, we can e.g. later add a check for n_fields, but there's no compelling reason to do it now, IMO. Also, [bpo-1820](https://bugs.python.org/issue1820) is still open, which is looking at the possibility of converging the structseq and namedtuple APIs.

tirkarthi
tirkarthi approved these changes Jan 1, 2020
View reviewed changes
Copy link
Member

@tirkarthi tirkarthi left a comment

Choose a reason for hiding this comment

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

Thanks @corona10 for bringing it up. I agree with @vsajip that namedtuple is more common and has a Python API with structseq used internally in posix and time module. If there is a report I guess we can add a similar check in future. PR LGTM. Thanks Vinay.

corona10
corona10 approved these changes Jan 1, 2020
View reviewed changes
Copy link
Member

@corona10 corona10 left a comment

Choose a reason for hiding this comment

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

@vsajip Thanks for the explanation :)
LGTM

@vsajip vsajip merged commit 46abfc1 into python:master Jan 1, 2020
@vsajip vsajip deleted the fix-39142 branch January 1, 2020 19:32
@miss-islington
Copy link
Contributor

Thanks @vsajip for the PR 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖

@bedevere-bot
Copy link

GH-17785 is a backport of this pull request to the 3.8 branch.

@miss-islington
Copy link
Contributor

Thanks @vsajip for the PR 🌮🎉.. I'm working now to backport this PR to: 3.7.
🐍🍒⛏🤖

@bedevere-bot
Copy link

GH-17786 is a backport of this pull request to the 3.7 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jan 1, 2020
@vsajip @miss-islington
bpo-39142: Avoid converting namedtuple instances to ConvertingTuple. (p…
d8f7fa3 
…ythonGH-17773)

This uses the heuristic of assuming a named tuple is a subclass of
tuple with a _fields attribute. This change means that contents of
a named tuple wouldn't be converted - if a user wants to have
ConvertingTuple functionality from a namedtuple, they will have to
implement it themselves.
(cherry picked from commit 46abfc1)

Co-authored-by: Vinay Sajip <vinay_sajip@yahoo.co.uk>
vsajip pushed a commit that referenced this pull request Jan 1, 2020
@miss-islington @vsajip
bpo-39142: Avoid converting namedtuple instances to ConvertingTuple. (G…
1d5a7e5 
…H-17773) (GH-17785)

(cherry picked from commit 46abfc1)
vsajip pushed a commit that referenced this pull request Jan 1, 2020
@miss-islington @vsajip
bpo-39142: Avoid converting namedtuple instances to ConvertingTuple. (G…
0e0e4ac 
…H-17773) (GH-17786)

(cherry picked from commit 46abfc1)
@bedevere-bot
Copy link

⚠️⚠️⚠️ Buildbot failure ⚠️⚠️⚠️

Hi! The buildbot x86-64 High Sierra 3.7 has failed when building commit 0e0e4ac.

What do you need to do:

  1. Don't panic.
  2. Check the buildbot page in the devguide if you don't know what the buildbots are or how they work.
  3. Go to the page of the buildbot that failed (https://buildbot.python.org/all/#builders/139/builds/34) and take a look at the build logs.
  4. Check if the failure is related to this commit (0e0e4ac) or if it is a false positive.
  5. If the failure is related to this commit, please, reflect that on the issue and make a new Pull Request with a fix.

You can take a look at the buildbot page here:

https://buildbot.python.org/all/#builders/139/builds/34

Summary of the results of the build (if available):

Click to see traceback logs 
remote: Enumerating objects: 14, done.        
remote: Counting objects:   7% (1/14)        
remote: Counting objects:  14% (2/14)        
remote: Counting objects:  21% (3/14)        
remote: Counting objects:  28% (4/14)        
remote: Counting objects:  35% (5/14)        
remote: Counting objects:  42% (6/14)        
remote: Counting objects:  50% (7/14)        
remote: Counting objects:  57% (8/14)        
remote: Counting objects:  64% (9/14)        
remote: Counting objects:  71% (10/14)        
remote: Counting objects:  78% (11/14)        
remote: Counting objects:  85% (12/14)        
remote: Counting objects:  92% (13/14)        
remote: Counting objects: 100% (14/14)        
remote: Counting objects: 100% (14/14), done.        
remote: Total 17 (delta 13), reused 13 (delta 13), pack-reused 3        
From https://github.com/python/cpython
 * branch                  3.7        -> FETCH_HEAD
Reset branch '3.7'

Configured with: --prefix=/Applications/Xcode.app/Contents/Developer/usr --with-gxx-include-dir=/usr/include/c++/4.2.1

/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: libpython3.7dm.a(dynamic_annotations.o) has no symbols
/Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ranlib: file: libpython3.7dm.a(pymath.o) has no symbols
ld: warning: text-based stub file /System/Library/Frameworks//CoreFoundation.framework/CoreFoundation.tbd and library file /System/Library/Frameworks//CoreFoundation.framework/CoreFoundation are out of sync. Falling back to library file for linking.
ld: warning: text-based stub file /System/Library/Frameworks//CoreFoundation.framework/CoreFoundation.tbd and library file /System/Library/Frameworks//CoreFoundation.framework/CoreFoundation are out of sync. Falling back to library file for linking.
rror: [Errno 2] No such file or directory: '/Users/buildbot/buildarea/3.7.billenstein-sierra/build/target/include/python3.7dm/pyconfig.h'
make: *** [sharedmods] Error 1

shihai1991 pushed a commit to shihai1991/cpython that referenced this pull request Jan 31, 2020
@vsajip @shihai1991
bpo-39142: Avoid converting namedtuple instances to ConvertingTuple. (p…
52d9077 
…ythonGH-17773)

This uses the heuristic of assuming a named tuple is a subclass of
tuple with a _fields attribute. This change means that contents of
a named tuple wouldn't be converted - if a user wants to have
ConvertingTuple functionality from a namedtuple, they will have to
implement it themselves.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tirkarthi tirkarthi tirkarthi approved these changes

@corona10 corona10 corona10 approved these changes

Assignees
No one assigned
Labels
type-bug An unexpected behavior, bug, or error
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@vsajip @miss-islington @bedevere-bot @tirkarthi @corona10 @the-knights-who-say-ni