Skip to content
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.

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

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

Open
wants to merge 19 commits into
base: main
Choose a base branch
from

Conversation

MarieRoald
Copy link

@MarieRoald MarieRoald commented Nov 3, 2024

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

Co-authored-by: Yngve Mardal Moe [email protected]


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

@bedevere-app

This comment was marked as outdated.

@Wulian233
Copy link
Contributor

LGTM
Recommend adding to what's new 3.14.rst

rruuaanng

This comment was marked as off-topic.

Copy link
Member

@Eclips4 Eclips4 left a comment

Choose a reason for hiding this comment

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

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

Doc/library/turtle.rst Outdated Show resolved Hide resolved
Doc/library/turtle.rst Outdated Show resolved Hide resolved
Automatically begin and end filling
-----------------------------------

If you have Python 3.14 or later, you don't need to call :func:`begin_fill` and
Copy link
Member

Choose a reason for hiding this comment

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

We don't usually include things like "If you have Python 3.14 or later" because each set of docs relates to one specific Python version.

But perhaps it's okay here because turtle is often used by beginners who might read these docs but have 3.12 or 3.13 installed?

Copy link
Contributor

Choose a reason for hiding this comment

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

That makes a lot of sense, but in this specific case I think it's useful. Many learners are at universities or schools with old versions of Python, and they might not be aware that there are several versions of Python. So I think it's useful to make it very clear that this is a new feature.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry to reopen, but IMO this reads a little bit unexpected. How about something in the lines of:

Starting with Python 3.14, you can use [...] instead of [...].

What do you think?

Doc/library/turtle.rst Show resolved Hide resolved
Lib/turtle.py Show resolved Hide resolved
Lib/turtle.py Outdated Show resolved Hide resolved
Lib/turtle.py Outdated Show resolved Hide resolved
Lib/turtle.py Outdated
@@ -110,27 +110,30 @@
from copy import deepcopy
from tkinter import simpledialog

from contextlib import contextmanager
Copy link
Contributor

@danielhollas danielhollas Nov 5, 2024

Choose a reason for hiding this comment

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

Why the extra newline between this and the previous import statement?

Doc/library/turtle.rst Outdated Show resolved Hide resolved
Co-authored-by: Hugo van Kemenade <[email protected]>
Co-authored-by: Daniel Hollas <[email protected]>
Doc/library/turtle.rst Outdated Show resolved Hide resolved
Doc/library/turtle.rst 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
Doc/library/turtle.rst Outdated Show resolved Hide resolved
Doc/library/turtle.rst Outdated Show resolved Hide resolved
Lib/test/test_turtle.py Outdated Show resolved Hide resolved
MarieRoald and others added 2 commits November 8, 2024 18:06
Doc/whatsnew/3.14.rst Outdated Show resolved Hide resolved
@yngvem
Copy link
Contributor

yngvem commented Nov 8, 2024

We have tried to address the review comments now 🙂

Copy link
Contributor

@picnixz picnixz left a comment

Choose a reason for hiding this comment

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

Some final minor nitpicks. Otherwise looks great!

Doc/whatsnew/3.14.rst Outdated Show resolved Hide resolved
Lib/test/test_turtle.py Outdated Show resolved Hide resolved
Lib/test/test_turtle.py Outdated Show resolved Hide resolved
Lib/turtle.py Outdated Show resolved Hide resolved
Lib/turtle.py Outdated Show resolved Hide resolved
Lib/test/test_turtle.py Outdated Show resolved Hide resolved
Lib/test/test_turtle.py Outdated Show resolved Hide resolved
Doc/library/turtle.rst Outdated Show resolved Hide resolved
MarieRoald and others added 3 commits November 9, 2024 05:39
Co-authored-by: Marie Roald <[email protected]>
Co-authored-by: Marie Roald <[email protected]>
Copy link
Contributor

@picnixz picnixz left a comment

Choose a reason for hiding this comment

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

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

Doc/library/turtle.rst Show resolved Hide resolved
Co-authored-by: Yngve Mardal Moe <[email protected]>
Copy link
Contributor

@picnixz picnixz left a comment

Choose a reason for hiding this comment

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

I promise there really shouldn't have anything left for me. Thank you for your patience! (my reviews tend to have multiple parts depending on the hour when I do the review).

Lib/turtle.py Outdated Show resolved Hide resolved
Co-authored-by: Bénédikt Tran <[email protected]>
@MarieRoald
Copy link
Author

I promise there really shouldn't have anything left for me. Thank you for your patience! (my reviews tend to have multiple parts depending on the hour when I do the review).

No worries, thanks a lot for the thorough review!

Copy link
Contributor

@erlend-aasland erlend-aasland left a comment

Choose a reason for hiding this comment

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

This looks good, but I have some remarks and nits. PTAL.

Automatically begin and end filling
-----------------------------------

If you have Python 3.14 or later, you don't need to call :func:`begin_fill` and
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry to reopen, but IMO this reads a little bit unexpected. How about something in the lines of:

Starting with Python 3.14, you can use [...] instead of [...].

What do you think?

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
Comment on lines 2007 to 2011
... dist = 2
... for i in range(200):
... fd(dist)
... rt(90)
... dist += 2
Copy link
Contributor

Choose a reason for hiding this comment

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

This can also be written more compact. Though, I'm not sure it is more readable, so feel free to ignore this :)

Suggested change
... dist = 2
... for i in range(200):
... fd(dist)
... rt(90)
... dist += 2
... for dist in range(0, 400, 2):
... fd(dist+2)
... rt(90)

Lib/turtle.py Outdated Show resolved Hide resolved
Lib/turtle.py Outdated
Comment on lines 3552 to 3558
"""Record the vertices of a polygon.

No argument.

Record the vertices of a polygon. Current turtle position is first point
of polygon. This function sets up a context manager that will
automatically end recording once exited.
Copy link
Contributor

Choose a reason for hiding this comment

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

Ditto.

MarieRoald and others added 3 commits November 14, 2024 05:44
Co-authored-by: Erlend E. Aasland <[email protected]>
Co-authored-by: Erlend E. Aasland <[email protected]>
Co-authored-by: Yngve Mardal Moe <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants