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-88773: Added teleport method to Turtle library #103974

Merged
merged 26 commits into from
Apr 30, 2023

Conversation

liam-gersten
Copy link
Contributor

@liam-gersten liam-gersten commented Apr 28, 2023

Fully implemented and tested a teleport method as described by the issue. This method behaves differently from the goto (or setpos) method, and can be especially helpful when creating and filling multiple separate objects.

Implementation:

There already exists a goto method, aliased as setpos | setposition | goto. It basically moves the turtle to the given position, but if the pen is down, it draws a line along the path. Additionally, the move is not instant, and you can see it go along the path to get to the destination. Users of the library may find themselves raising the pen, calling the method, and then lowering it so that a line doesn’t get drawn. Another issue with using goto for this is that regardless of whether the pen is raised, an "imaginary" line is still drawn from the current position to the destination. When filling is enabled, this invisible line acts as a boundary for the fill color. If one were creating multiple separate objects with filling enabled, they would need to run the exact code in the teleport method in order to fill them separately as desired. The following is a demo of the difference between goto and teleport for filling multiple objects.

Justification:

One might think we should just update the goto method to behave this way with a flag like teleport=True. However, all of the pen controlling methods are in one class, TPen, and all of the visual and position controlling methods are in another, TNavigator. The primary turtle class, RawTurtle(TPen, TNavigator) inherits methods from both of these, and since teleport needs access to methods and attributes in both of these classes, this is where it should be written. While it would be possible to give goto access to the TPen class via inheritance, this would be a messy refactor for what should be a distinct method.

Resolves #88773

@bedevere-bot

This comment was marked as resolved.

@cpython-cla-bot
Copy link

cpython-cla-bot bot commented Apr 28, 2023

All commit authors signed the Contributor License Agreement.
CLA signed

@arhadthedev arhadthedev added stdlib Python modules in the Lib dir topic-tkinter labels Apr 28, 2023
@gpshead gpshead self-assigned this Apr 28, 2023
@gpshead gpshead added the type-feature A feature request or enhancement label Apr 28, 2023
Copy link
Member

@terryjreedy terryjreedy left a comment

Choose a reason for hiding this comment

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

I cannot comment on the desirability of the addition until I have time to test and verify justification assertion. Not now.
Nor have I reviewed the new tkinter code.

Blurb needs expansion. I will commit my proposal and you can propose further revision.

Doc and test additions are needed before merge.

Note: Topics are for issues. Core developers get notified of PRs by signing up as 'code owner' of specific files. Besides which, this does not touch tkinter.

@bedevere-bot
Copy link

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

Copy link
Member

@gpshead gpshead left a comment

Choose a reason for hiding this comment

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

Please also document this in Doc/library/turtle.rst. Be sure to include a .. versionadded: 3.12 annotation on the new teleport method documentation in there.

Lib/turtle.py Outdated Show resolved Hide resolved
@python python deleted a comment from bedevere-bot Apr 28, 2023
@Fidget-Spinner
Copy link
Member

Fidget-Spinner commented Apr 28, 2023

Terry and Gregory, thank you for taking the time to review this PR.

If you're wondering where to add tests, they are found in /Lib/test/. In this case, it is test_turtle.py. You can run the test with ./python -m test test_turtle. Read the devguide here to learn more https://devguide.python.org/testing/run-write-tests/ .

Documentation help can be found here https://devguide.python.org/documentation/ .

I also sent you an email to discuss CPython contribution. Please take a look when you have the time. Thanks!

Doc/library/turtle.rst Outdated Show resolved Hide resolved
Lib/turtle.py Outdated Show resolved Hide resolved
@bedevere-bot
Copy link

Thanks for making the requested changes!

@terryjreedy, @gpshead: please review the changes made to this pull request.

liam-gersten

This comment was marked as outdated.

Lib/test/test_turtle.py Outdated Show resolved Hide resolved
@gvanrossum
Copy link
Member

I wish the turtle module was a 3rd party package, but since it isn't, and is actively used, I think this is a good contribution. I hope @gpshead will come through with his promise of review of the PR. (I didn't see anything objectionable on a quick skim.)

@liam-gersten
Copy link
Contributor Author

I have made the requested changes; please review again.

@bedevere-bot
Copy link

Thanks for making the requested changes!

@gpshead, @terryjreedy: please review the changes made to this pull request.

@gpshead gpshead merged commit 74a2b79 into python:main Apr 30, 2023
carljm added a commit to carljm/cpython that referenced this pull request May 1, 2023
* main: (26 commits)
  pythongh-104028: Reduce object creation while calling callback function from gc (pythongh-104030)
  pythongh-104036: Fix direct invocation of test_typing (python#104037)
  pythongh-102213: Optimize the performance of `__getattr__` (pythonGH-103761)
  pythongh-103895: Improve how invalid `Exception.__notes__` are displayed (python#103897)
  Adjust expression from `==` to `!=` in alignment with the meaning of the paragraph. (pythonGH-104021)
  pythongh-88496: Fix IDLE test hang on macOS (python#104025)
  Improve int test coverage (python#104024)
  pythongh-88773: Added teleport method to Turtle library (python#103974)
  pythongh-104015: Fix direct invocation of `test_dataclasses` (python#104017)
  pythongh-104012: Ensure test_calendar.CalendarTestCase.test_deprecation_warning consistently passes (python#104014)
  pythongh-103977: compile re expressions in platform.py only if required (python#103981)
  pythongh-98003: Inline call frames for CALL_FUNCTION_EX (pythonGH-98004)
  Replace Netlify with Read the Docs build previews (python#103843)
  Update name in acknowledgements and add mailmap (python#103696)
  pythongh-82054: allow test runner to split test_asyncio to execute in parallel by sharding. (python#103927)
  Remove non-existing tools from Sundry skiplist (python#103991)
  pythongh-103793: Defer formatting task name (python#103767)
  pythongh-87092: change assembler to use instruction sequence instead of CFG (python#103933)
  pythongh-103636: issue warning for deprecated calendar constants (python#103833)
  Various small fixes to dis docs (python#103923)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
stdlib Python modules in the Lib dir type-feature A feature request or enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Teleport method for turtle class
8 participants