GeezerVison

gpshead self-assigned this

gpshead requested changes

View reviewed changes

gpshead left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

likely good, but i'll take a closer look tomorrow. one suggestion added.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Lib/turtle.py Resolved

Uh oh!

There was an error while loading. Please reload this page.

bedevere-bot added awaiting changes and removed awaiting review labels

bedevere-bot commented Aug 10, 2023

Copy link

Copy Markdown

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.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

sobolevn added 2 commits

August 10, 2023 09:42


          Use patma

43f131f


          Better error message

51f5bf0

serhiy-storchaka approved these changes

Aug 11, 2023

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting changes labels

Aug 11, 2023

gpshead reviewed

View reviewed changes

Comment thread

Lib/test/test_turtle.py Outdated

		'pen': '(pen=None, **pendict)',
		}

		for name in turtle.__all__:

gpshead Aug 10, 2023

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

The purpose of this test is only to verify known signatures, correct?

In that case, looping over known_signatures.items() rather than turtle.__all__ makes more sense. Otherwise a typo in a known_signatures name key would go silently unnoticed and we're otherwise creating subtests for all names in turtle even though most of them don't test anything.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

serhiy-storchaka requested changes

View reviewed changes

serhiy-storchaka left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Well, I think that it would be nice to add also tests for the original issue:

Call teleport() without arguments.
Call teleport() with three positional arguments (it should fail).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

bedevere-bot removed the awaiting merge label

bedevere-bot commented Aug 18, 2023

Copy link

Copy Markdown

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bedevere-bot added the awaiting changes label


          Address review

438dcbc

sobolevn commented Aug 21, 2023

Copy link

Copy Markdown

Member Author

I've addressed @gpshead comments about the test case, but I don't think that I can actually test these functions:

Nothing similar is tested in test_turtle, it simply does not have any Turtle class tests at all
I don't know anything about tkinter
I don't really think that we should add test foundation for it in this specific PR
inspect works with real python function signature, so it is at least something

If anyone wants to contribute some tkinter-based tests for Turtle class, I would be happy to include teleport test there 👍

gpshead reacted with thumbs up emoji

All reactions

👍 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          Merge branch 'main' into issue-107805

ac50e7f

serhiy-storchaka approved these changes

Aug 27, 2023

View reviewed changes

serhiy-storchaka left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

Well, testing module-level turtle function is a non-trivial task.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

bedevere-bot added awaiting merge and removed awaiting changes labels

Aug 27, 2023

gpshead approved these changes

View reviewed changes

gpshead left a comment

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

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

I manually tested that turtle.Turtle() appears to work and that Turtle.teleport works.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

gpshead merged commit 044b8b3 into python:main

miss-islington commented Sep 1, 2023

Copy link

Copy Markdown

Contributor

Thanks @sobolevn for the PR, and @gpshead for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bedevere-bot removed the awaiting merge label

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request


          pythongh-107805: Fix signatures of module-level generated functions i…

712dede

…n `turtle` (pythonGH-107807)

(cherry picked from commit 044b8b3)

Co-authored-by: Nikita Sobolev <[email protected]>
Co-authored-by: Alex Waygood <[email protected]>

bedevere-bot commented Sep 1, 2023

Copy link

Copy Markdown

GH-108749 is a backport of this pull request to the 3.12 branch.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

bedevere-bot removed the needs backport to 3.12 only security fixes label

Yhg1s pushed a commit that referenced this pull request