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

MarieRoald · 2024-11-03T05:59:33Z

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>

Wulian233 · 2024-11-03T09:14:54Z

LGTM
Recommend adding to what's new 3.14.rst

Lib/turtle.py

Eclips4

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

Doc/library/turtle.rst

Lib/turtle.py

Lib/test/test_turtle.py

Lib/turtle.py

Doc/library/turtle.rst

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

Doc/library/turtle.rst

Lib/test/test_turtle.py

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

Doc/whatsnew/3.14.rst

yngvem · 2024-11-08T19:00:54Z

We have tried to address the review comments now 🙂

picnixz

Some final minor nitpicks. Otherwise looks great!

Doc/whatsnew/3.14.rst

Lib/test/test_turtle.py

Lib/turtle.py

Lib/test/test_turtle.py

Doc/library/turtle.rst

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

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

picnixz

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

Doc/library/turtle.rst

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

hugovk · 2024-12-29T13:36:26Z

(Conflict resolved.)

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

…ntry

Lib/test/test_turtle.py

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

Lib/test/test_turtle.py

graingert

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

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

yngvem · 2025-01-18T05:37:26Z

We've addressed the review comments now 🙂

Remark was addressed

erlend-aasland

Thanks, everyone! :)

vstinner · 2025-01-19T14:04:28Z

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

vstinner · 2025-01-19T14:11:51Z

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

yngvem · 2025-01-20T04:30:07Z

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:?

hugovk · 2025-01-20T06:05:46Z

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.

yngvem · 2025-01-20T07:20:15Z

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

hugovk · 2025-01-20T16:20:35Z

@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

Lib/turtle.py Show resolved Hide resolved

Eclips4 reviewed Nov 3, 2024

View reviewed changes

hugovk reviewed Nov 3, 2024

View reviewed changes

Doc/library/turtle.rst Outdated Show resolved Hide resolved

Doc/library/turtle.rst Outdated Show resolved Hide resolved

Doc/library/turtle.rst Outdated Show resolved Hide resolved

Doc/library/turtle.rst Show resolved Hide resolved

Lib/turtle.py Show resolved Hide resolved

erlend-aasland reviewed Nov 4, 2024

View reviewed changes

Lib/test/test_turtle.py Outdated Show resolved Hide resolved

danielhollas reviewed Nov 5, 2024

View reviewed changes

Lib/turtle.py Outdated Show resolved Hide resolved

Lib/turtle.py Outdated Show resolved Hide resolved

Lib/turtle.py Outdated Show resolved Hide resolved

Doc/library/turtle.rst Outdated Show resolved Hide resolved

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

Doc/whatsnew/3.14.rst Outdated Show resolved Hide resolved

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

Doc/library/turtle.rst Show resolved Hide resolved

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

Lib/test/test_turtle.py Outdated Show resolved Hide resolved

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

Lib/test/test_turtle.py Outdated Show resolved Hide resolved

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
38 checks passed

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

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 #126350

Uh oh!

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!

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

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

MarieRoald commented Nov 3, 2024 •

edited by hugovk

Loading

picnixz left a comment •

edited

Loading

vstinner commented Jan 19, 2025 •

edited

Loading