◐ Shell
reader mode source ↗
Skip to content

bpo-11572: Make several minor improvements to copy module#207

Closed
berkerpeksag wants to merge 1 commit into
python:masterfrom
berkerpeksag:11572-copy-improvements
Closed

bpo-11572: Make several minor improvements to copy module#207
berkerpeksag wants to merge 1 commit into
python:masterfrom
berkerpeksag:11572-copy-improvements

Conversation

@berkerpeksag

@berkerpeksag berkerpeksag commented Feb 21, 2017 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Member
  • When doing getattr lookups with a default of "None", it
    now uses an "is" comparison against None which is both
    faster and more correct
  • Used getattr() instead of hasattr()
  • Removed obsolete branches where it's impossible to be
    called in Python 3

Patch by Brandon Rhodes.

https://bugs.python.org/issue11572

@berkerpeksag berkerpeksag added the type-feature A feature request or enhancement label Feb 21, 2017
@berkerpeksag

berkerpeksag commented Feb 21, 2017

Copy link
Copy Markdown
Member Author

Another possible cleanup opportunity (added in https://hg.python.org/cpython/rev/c4715c9f442f)

try:
    issc = issubclass(cls, type)
except TypeError: # cls is not a class
    issc = False
if issc:
    # treat it as a regular class:
    return _copy_immutable(x)

@brettcannon

brettcannon commented Feb 21, 2018

Copy link
Copy Markdown
Member

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.

@berkerpeksag berkerpeksag force-pushed the 11572-copy-improvements branch from 569fdda to 90d74b3 Compare June 14, 2018 00:44
@berkerpeksag

berkerpeksag commented Jun 14, 2018

Copy link
Copy Markdown
Member Author

Thanks for the review, Serhiy. I've addressed all of your review comments except the ones about x.__reduce_ex__.

Also, I still need to a NEWS entry.

@berkerpeksag

berkerpeksag commented Jul 9, 2018

Copy link
Copy Markdown
Member Author

@serhiy-storchaka can you take a look at the latest version of this PR?

serhiy-storchaka
serhiy-storchaka reviewed Jul 9, 2018
View reviewed changes

@serhiy-storchaka serhiy-storchaka left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hide comment

The PR contains not just cleanup, but leads to some user-visible behavior changes. They are not consistent with pickle implementations, and it is not obvious that they are purposed. I suggest to revert these changes, and open a separate issue for discussing about the meaning of setting None to special methods like __reduce_ex__, __getstate__ and __setstate__.

@berkerpeksag berkerpeksag force-pushed the 11572-copy-improvements branch from 90d74b3 to 3b49327 Compare July 9, 2018 17:59
@berkerpeksag

berkerpeksag commented Jul 9, 2018

Copy link
Copy Markdown
Member Author

@serhiy-storchaka thanks for the review! Reverted them in 3b49327. Do you think we need a NEWS entry after 3b49327?

@serhiy-storchaka

serhiy-storchaka commented Jul 9, 2018

Copy link
Copy Markdown
Member

Thank you @berkerpeksag. I have no opinion about the NEWS entry.

@berkerpeksag

berkerpeksag commented Jul 9, 2018

Copy link
Copy Markdown
Member Author

Ok, I'm not going to add a NEWS entry since it's just a cleanup PR.

@berkerpeksag
bpo-11572: Make several minor improvements to copy module …
0a88fde 
* When doing getattr lookups with a default of "None", it
  now uses an "is" comparison against None which is more
  correct
* Removed outdated code

Patch by Brandon Rhodes.
@berkerpeksag berkerpeksag force-pushed the 11572-copy-improvements branch from 3b49327 to 0a88fde Compare July 9, 2018 19:31
@berkerpeksag berkerpeksag dismissed serhiy-storchaka’s stale review July 9, 2018 19:33

Addressed Serhiy's review comments.

@berkerpeksag

berkerpeksag commented Jul 9, 2018

Copy link
Copy Markdown
Member Author

@serhiy-storchaka do you know what the problem with the build? I can't reproduce it locally.

Fixing C file whitespace ... 1 file:
  Modules/unicodedata_db.h
Fixing docs whitespace ... 0 files
Please fix the 1 file(s) with whitespace issues

It looks like something is wrong with the patchcheck script.

@berkerpeksag berkerpeksag deleted the 11572-copy-improvements branch July 9, 2018 20:14
akruis added a commit to akruis/cpython that referenced this pull request Jan 16, 2019
@akruis
Stackless issue python#207: Empty obsolete header files …
a756c79 
cframeobject.h and channelobject.h are now empty and obsolete.
They will be removed by the next commit.
akruis added a commit to akruis/cpython that referenced this pull request Jan 16, 2019
@akruis
Stackless issue python#207: Move/rename/remove Stackless header files …
2d7c76c 
Move the headers to Include and Include/internal, fix the #include
directives and update Makefile.in.pre and PCbuild-files.
akruis added a commit to akruis/cpython that referenced this pull request Jan 16, 2019
@akruis
Stackless issue python#207: Remove the Stackless include dirs from To… …
3f48801 
…ols/msi

Remove the special handling of Stackless include files from the
msi-installer.
akruis added a commit to akruis/cpython that referenced this pull request Jan 16, 2019
@akruis
Stackless issue python#207: Update the Mac Makefile.in …
46106ce 
Remove the Stackless source dir from the include-path.
akruis added a commit to akruis/cpython that referenced this pull request Jan 16, 2019
@akruis
Stackless issue python#207: Update distutils …
71bb55a 
Remove the Stackless specific include-path.
akruis added a commit to akruis/cpython that referenced this pull request Jan 18, 2019
@akruis
Stackless issue python#207: Minor fix of PCbuild files
45af12e
akruis added a commit to akruis/cpython that referenced this pull request Jan 18, 2019
@akruis
Stackless issue python#207: don't include internal headers from publi… …
2eee330 
…c headers

Move the definition of the macro SLP_SEH32 to stackless_struct.h
akruis added a commit to akruis/cpython that referenced this pull request Jan 18, 2019
@akruis
Stackless issue python#207: consistent header files names …
90b1e00 
Names of "public" header files use prefix "stackless", all other
use "slp". This is consistent with symbol names.
akruis added a commit to akruis/cpython that referenced this pull request Jan 18, 2019
@akruis
Stackless issue python#207: add changelog entry
b06944e
jaraco pushed a commit that referenced this pull request Dec 2, 2022
@pyup-bot @Mariatta
Update pytest from 4.2.0 to 4.2.1 (#207)
fd02e8b
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@serhiy-storchaka serhiy-storchaka serhiy-storchaka left review comments

Assignees

No one assigned

Labels

awaiting changes skip news type-feature A feature request or enhancement

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@berkerpeksag @brettcannon @serhiy-storchaka @the-knights-who-say-ni