FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

bpo-43988: Add test.support.check_disallow_instantiation by erlend-aasland · Pull Request #25757 · python/cpython · GitHub

/ cpython Public

bpo-43988: Add test.support.check_disallow_instantiation - #25757

Merged
vstinner merged 1 commit into
python:mainfrom
erlend-aasland:test-support
May 26, 2021
ConversationCommits (1)ChecksFiles changed
Merged

bpo-43988: Add test.support.check_disallow_instantiation#25757
vstinner merged 1 commit into
python:mainfrom
erlend-aasland:test-support

Conversation

erlend-aasland commented Apr 30, 2021 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Contributor

erlend-aasland commented Apr 30, 2021

Copy link
Copy Markdown
Contributor Author

cc. @vstinner

vstinner reviewed Apr 30, 2021
View reviewed changes
Comment thread Lib/test/support/__init__.py

See bpo-43916.
"""
msg = f"cannot create '{tp.__module__}\.{tp.__name__}' instances"

vstinner Apr 30, 2021

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.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

You should use f"xxx\.xxx" or fr"xxx.xxx". Or maybe use re.escape().

erlend-aasland May 2, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Since these tests are decorated with cpython_only, it should be ok to match the exact string, as produced by Objects/typeobject.c? Just matching the type module/name could in theory generate wrong result; maybe not in practice though.

erlend-aasland May 2, 2021

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

What about setting the match pattern depending on what test.support.check_impl_detail() says?

Comment thread Lib/test/support/__init__.py
vstinner reviewed Apr 30, 2021
View reviewed changes
Comment thread Lib/test/support/__init__.py
vstinner reviewed Apr 30, 2021
View reviewed changes
Comment thread Lib/test/support/__init__.py
vstinner merged commit 4f72526 into python:main May 26, 2021

vstinner commented May 26, 2021

Copy link
Copy Markdown
Member

Enough nitpicking, you can always enhance the code later ;-) I merged your PR, thanks. I guess that the next step is to modify existing tests to use it.

erlend-aasland deleted the test-support branch May 26, 2021 14:40
erlend-aasland mentioned this pull request May 26, 2021
Merged

erlend-aasland commented May 26, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Backport to 3.10? I guess not.

vstinner commented May 26, 2021

Copy link
Copy Markdown
Member

Backport to 3.10? I guess not.

It seems like more changes are coming. Once they all landed into main, maybe write a backport to 3.10 PR including all of them at once (git cherry-pick -x all commits in the right order). Or we can leave 3.10 as it is: it's more up to you.

erlend-aasland commented May 26, 2021

Copy link
Copy Markdown
Contributor Author

Lets leave 3.10 as it is.

erlend-aasland mentioned this pull request Jun 20, 2021
Closed

erlend-aasland commented Jun 20, 2021 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@pablogsal, should we backport this? grep -r disallow_insta Lib/test | wc -l gives me 14 hits on the 3.10 branch, including the recently backported sqlite3 test.

pablogsal commented Jun 20, 2021

Copy link
Copy Markdown
Member

@pablogsal, should we backport this? grep -r disallow_insta Lib/test | wc -l gives me 14 hits on the 3.10 branch, including the recently backported sqlite3 test.

I would prefer to backport it, yep

erlend-aasland pushed a commit to erlend-aasland/cpython that referenced this pull request Jun 23, 2021
[3.10] bpo-43988: Add test.support.check_disallow_instantiation() (py…
9122e87 
…thonGH-25757).

(cherry picked from commit 4f72526)

Co-authored-by: Erlend Egeberg Aasland <erlend.aasland@innova.no>

erlend-aasland commented Jun 23, 2021

Copy link
Copy Markdown
Contributor Author

Once they all landed into main, maybe write a backport to 3.10 PR including all of them at once (git cherry-pick -x all commits in the right order)

@vstinner: I've opened GH-26885 which is a backport of this PR, GH-26394 (docs for this PR), and GH-26392 (apply this PR to Lib/test).

miss-islington pushed a commit that referenced this pull request Jun 23, 2021
[3.10] bpo-43988: Add test.support.check_disallow_instantiation() (GH…
0a3452e 
…-25757) (GH-26885)

(cherry picked from commit 4f72526, fbff538, and 8cec740)

Co-authored-by: Erlend Egeberg Aasland <erlend.aasland@innova.no>

Automerge-Triggered-By: GH:vstinner
pablogsal mentioned this pull request Jul 18, 2021
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

vstinner vstinner left review comments

Assignees

No one assigned

Labels

skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

Back | FazBrowse Home | New Git URL