gh-126349: Add context managers to turtle for `fill`, `poly` and `no_animation` by MarieRoald · Pull Request #126350 · python/cpython · GitHub

Adds fill(), poly() and no_animation() context managers to turtle.py.

Co-authored-by: Yngve Mardal Moe 3531982+yngvem@users.noreply.github.com

Issue: Turtle context managers for fill, poly and tracer #126349

📚 Documentation preview 📚: https://cpython-previews--126350.org.readthedocs.build/en/126350/library/turtle.html

Co-authored-by: Marie Roald <roald.marie@gmail.com>

Co-authored-by: Yngve Mardal Moe <yngve.m.moe@gmail.com>

Co-authored-by: Marie Roald <roald.marie@gmail.com>

Co-authored-by: Yngve Mardal Moe <yngve.m.moe@gmail.com>

LGTM
Recommend adding to what's new 3.14.rst

Could you please add a note about these additions to the Doc/whatsnew/3.14.rst?

Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com> Co-authored-by: Daniel Hollas <danekhollas@gmail.com>

Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com> Co-authored-by: Daniel Hollas <danekhollas@gmail.com>

We have tried to address the review comments now 🙂

Some final minor nitpicks. Otherwise looks great!

Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>

Co-authored-by: Marie Roald <roald.marie@gmail.com>

Last one and I am good! (modulo Hugo's comment)

Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>

(Conflict resolved.)

@erlend-aasland Any more comments or good to merge?

…ntry

Co-authored-by: Yngve Mardal Moe <yngve.m.moe@gmail.com>

the mock.patch context manager handling in the tests could be improved

Co-authored-by: Yngve Mardal Moe <yngve.m.moe@gmail.com>

We've addressed the review comments now 🙂

Remark was addressed

Thanks, everyone! :)

This change introduced a reference leak in test_turtle:

$ ./python -m test test_turtle -R 3:3
...
test_turtle leaked [13847, 13843, 13847] references, sum=41537
test_turtle leaked [3399, 3397, 3399] memory blocks, sum=10195
...

cc @encukou

I suspect that the regression comes from the new patch_screen() function in test_turtle.

I believe we found a fix. If we add the following teardown method to TestTurtle

    def tearDown(self):
        turtle.Turtle._screen = None
        return super().tearDown()

then @vstinner's command gives this output instead:

[...]
beginning 6 repetitions. Showing number of leaks (. for 0 or less, X for 10 or more)
123:456
XX. ...

== Tests result: SUCCESS ==
[...]

We're not sure how the workflow is for fixing regressions that we introduced. Should we make a new PR that starts with gh-126349:?

Should we make a new PR that starts with gh-126349:?

Yes please, because we've only just merged this PR, the fix can go under the same issue.

We'll submit a PR when we're off work today then :)

@encukou has created a fix at #129079.

…ers to turtle (python#126350) Co-authored-by: Marie Roald <roald.marie@gmail.com> Co-authored-by: Yngve Mardal Moe <yngve.m.moe@gmail.com> Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com> Co-authored-by: Daniel Hollas <danekhollas@gmail.com> Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com> Co-authored-by: Erlend E. Aasland <erlend@python.org>

yngvem and others added 5 commits November 3, 2024 05:21

Add context managers for turtle.fill and turtle.poly

3379b3d

Co-authored-by: Marie Roald <roald.marie@gmail.com>

Add documentation

879d886

Co-authored-by: Yngve Mardal Moe <yngve.m.moe@gmail.com>

Add context manager to turtle.TurtleScreen for disabling auto-update

54f709c

Co-authored-by: Yngve Mardal Moe <yngve.m.moe@gmail.com>

Forward functions correctly

437c8ce

Co-authored-by: Marie Roald <roald.marie@gmail.com>

Add documentation

d11d8f9

Co-authored-by: Marie Roald <roald.marie@gmail.com>

This comment was marked as outdated.

Sign in to view

bedevere-app Bot added the awaiting review label Nov 3, 2024

bedevere-app Bot mentioned this pull request Nov 3, 2024

Turtle context managers for fill, poly and tracer #126349

Closed

blurb-it Bot and others added 2 commits November 3, 2024 06:05

📜🤖 Added by blurb_it.

1f04ee4

Fix typo in docs

3947de1

Co-authored-by: Yngve Mardal Moe <yngve.m.moe@gmail.com>

This comment was marked as off-topic.

Sign in to view

nineteendo reviewed Nov 3, 2024

View reviewed changes

Comment thread Lib/turtle.py

Eclips4 reviewed Nov 3, 2024

View reviewed changes

hugovk reviewed Nov 3, 2024

View reviewed changes

Comment thread Doc/library/turtle.rst Outdated

Comment thread Doc/library/turtle.rst Outdated

Comment thread Doc/library/turtle.rst Outdated

Comment thread Doc/library/turtle.rst

Comment thread Lib/turtle.py

erlend-aasland reviewed Nov 4, 2024

View reviewed changes

Comment thread Lib/test/test_turtle.py Outdated

danielhollas reviewed Nov 5, 2024

View reviewed changes

Comment thread Lib/turtle.py Outdated

Comment thread Lib/turtle.py Outdated

Comment thread Lib/turtle.py Outdated

Comment thread Doc/library/turtle.rst Outdated

Apply suggestions from code review

5ee489b

Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com> Co-authored-by: Daniel Hollas <danekhollas@gmail.com>

picnixz reviewed Nov 8, 2024

View reviewed changes

MarieRoald and others added 2 commits November 8, 2024 18:06

Address review comments

28a5ac6

Apply suggestions from code review

6621bd3

Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com> Co-authored-by: Daniel Hollas <danekhollas@gmail.com>

picnixz reviewed Nov 8, 2024

View reviewed changes

Comment thread Doc/whatsnew/3.14.rst Outdated

Address review comments

2f5c488

picnixz reviewed Nov 8, 2024

View reviewed changes

MarieRoald and others added 3 commits November 9, 2024 05:39

Apply suggestions from code review

302a1ed

Co-authored-by: Bénédikt Tran <10796600+picnixz@users.noreply.github.com>

Use setUp for unit tests

ad779d5

Co-authored-by: Marie Roald <roald.marie@gmail.com>

Address review comments

fd89c95

Co-authored-by: Marie Roald <roald.marie@gmail.com>

picnixz approved these changes Nov 9, 2024

View reviewed changes

Comment thread Doc/library/turtle.rst

bedevere-app Bot removed the awaiting review label Nov 9, 2024

bedevere-app Bot added awaiting merge and removed awaiting core review labels Dec 17, 2024

MarieRoald and others added 3 commits December 18, 2024 07:09

Update Lib/test/test_turtle.py

815a8a3

Co-authored-by: Hugo van Kemenade <1324225+hugovk@users.noreply.github.com>

Merge branch 'main' into fix-issue-126349

fc01695

Re-sort

8487336

erlend-aasland added 3 commits December 30, 2024 12:21

Merge branch 'main' into fix-issue-126349

401c07f

Pull in main and resolve conflicts

3f2b4ef

Remove unneeded stylistic change; remove unprintable char from NEWS e…

0c2fca0

…ntry

erlend-aasland reviewed Dec 30, 2024

View reviewed changes

Comment thread Lib/test/test_turtle.py Outdated

erlend-aasland changed the title gh-126349 Add context managers to turtle for fill, poly and no_animation gh-126349: Add context managers to turtle for fill, poly and no_animation Dec 30, 2024

Change assert to self.assertEqual

6115eef

Co-authored-by: Yngve Mardal Moe <yngve.m.moe@gmail.com>

graingert reviewed Jan 6, 2025

View reviewed changes

Comment thread Lib/test/test_turtle.py Outdated

graingert previously requested changes Jan 6, 2025

View reviewed changes

MarieRoald and others added 4 commits January 18, 2025 04:56

Change assert to self.assertEqual

11d9bc3

Co-authored-by: Yngve Mardal Moe <yngve.m.moe@gmail.com>

Improve usage of unittest.mock.patch

061fe2a

Co-authored-by: Yngve Mardal Moe <yngve.m.moe@gmail.com>

Merge branch 'main' into fix-issue-126349

32bbf68

Merge branch 'main' into fix-issue-126349

b2ab84b

erlend-aasland approved these changes Jan 18, 2025

View reviewed changes

erlend-aasland merged commit d3adf02 into python:main Jan 18, 2025

bedevere-app Bot removed the awaiting merge label Jan 18, 2025

gh-126349: Add context managers to turtle for `fill`, `poly` and `no_animation` by MarieRoald · Pull Request #126350 · python/cpython · GitHub

Uh oh!

Uh oh!

gh-126349: Add context managers to turtle for fill, poly and no_animation - #126350

gh-126349: Add context managers to turtle for fill, poly and no_animation#126350erlend-aasland merged 30 commits intopython:mainpython/cpython:mainfrom MarieRoald:fix-issue-126349MarieRoald/cpython:fix-issue-126349Copy head branch name to clipboard

Conversation

MarieRoald commented Nov 3, 2024 • edited by hugovk Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

This comment was marked as outdated.

Wulian233 commented Nov 3, 2024

Uh oh!

This comment was marked as off-topic.

Uh oh!

Uh oh!

Eclips4 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

yngvem commented Nov 8, 2024

Uh oh!

picnixz left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

picnixz left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hugovk commented Dec 29, 2024

Uh oh!

Uh oh!

Uh oh!

graingert left a comment

Choose a reason for hiding this comment

Uh oh!

yngvem commented Jan 18, 2025

Uh oh!

erlend-aasland left a comment

Choose a reason for hiding this comment

Uh oh!

vstinner commented Jan 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vstinner commented Jan 19, 2025

Uh oh!

yngvem commented Jan 20, 2025

Uh oh!

hugovk commented Jan 20, 2025

Uh oh!

yngvem commented Jan 20, 2025

Uh oh!

hugovk commented Jan 20, 2025

Uh oh!

gh-126349: Add context managers to turtle for fill, poly and no_animation#126350
erlend-aasland merged 30 commits into
python:mainfrom
MarieRoald:fix-issue-126349

MarieRoald commented Nov 3, 2024 •
edited by hugovk
Loading

picnixz left a comment •
edited
Loading

vstinner commented Jan 19, 2025 •
edited
Loading