Clean up turtle.py code formatting #65772
Comments
|
Lib/turtle.py has some code formatting issues. Let's clean them up to make the module easier to read as interns start working on it this summer. Specifically:
|
|
I'm claiming this ticket. Plan to work on it during my OPW internship. |
|
I am glad to see this. Gregor has been inactive for several years, Except for Ned's recent patch for OSX, it has been years since there was a turtle-specific code patch (from hg revision history). Questions: is there project link? are any of the mentors core developers, with commit rights? or would you need commits from someone like me? I have read turtle.py and found that the multiple layers made some things hard to follow. To me, this was a bigger impediment than code formatting. You may want to develop an outline of the layer structure. As you may know, Guido generally discourages pure code cleanups and prefers that they be done when the code is examined for other purposes. I personally think some changes are safe enough if verified by a second person. I looked for and did not fine test/test_turtle. Did I miss something? Turtledemo is a partial substitute, but it might not exercise all turtle functions. If you only apply cleanups to 3.5, subsequent bug fixes for all 3 versions will become much more difficult. The default commit process assumes that maintenance patches merge forward cleanly. I try to keep Idle code the same across versions as much as possible. |
|
Hi Terry,
I am not 100% sure. Let me ask Jessica, who is my mentor, and get back to you.
There is no project link to Turtle cleanup specifically. But I can definitely try to reorganize the code such that it flows better and it is easier to read. I can add that to the list.
I actually did not know that Guido discourages pure code cleanup. Should I try to find a feature to add? Currently, I am doing all the easy stuff, and making it PEP-8 compliant and work through linter. I am also trying to delete some of the commented out code.
I actually don't see the Turtle tests as well. Should creating unit tests for turtle.py be a separate ticket? I am currently developing off of 3.4. I will try to run my patch through all the versions of Python 3 to make sure it is a clean fix. |
|
When I re-read the top of the file, I remembered that turtle.py has unique issues in relation to code ownership. (I was once told that Gregor had to approve any substantive code change.) I asked about that and about code cleanups on pydev (thread 'updating turtle.py'). Re your message: I was suggesting 'mapping' the code, not re-structuring it. Testing: A complete 'unit' test would test each function in each layer. A minimal 'unit' test should at least test each top-level function and check response on the canvas. Assuming that one can get to tk root and canvas, some things should be possible. But I don't know what introspection functions a canvas has. An alternative would be to replace the canvas with a mock-canvas with extra introspection added. Another alternative would be a human-verified test, a turtle script that systematically called every function and said that it was doing for a person to verify. "Line width: 1, 2, 3, 5, 8, 12 17, 30" (with a slight pause for each width). Versions: at most 2.7, 3.4, 3.5. The 3.4 and 3.5 turtle code should be close to identical. |
|
Terry, thank you for all the time you've been putting into the GSoC and OPW tickets.
https://wiki.python.org/moin/OPW/2014#Graphical_Python is a broad-strokes outline. As we get further into the internship we'll decide on areas of focus. I'm the main mentor. I don't have commit rights but would be reviewing most of the changes before putting them in the commit review stage.
You didn't miss anything. :) Part of this internship will be adding unit test coverage. @lita: I'll take care of creating unit test tickets that split up the work between you and Ingrid.
Ingrid Cheung (added to the nosy list) is working on unit test scaffolding, inspired by the tkinter tests. I want to make sure there's a clear course of action for Lita on this ticket. If cleanup is controversial, how about rescoping this to points (3) and (4) from the original ticket statement:
@terry, what do you think about that? @lita, your PEP-8 and linter work has not been in vain. :) It'll come in handy local to where you fix bugs and add features down the road. |
|
I like the proposal and would like to see it happen. My concern is to avoid having interns write patches that get rejected for non-technical reasons. I won't make any specific suggestions until I get more information either from the pydev thread Lita, please post a summary of the types of issues you have found (at most, say, 20). Some things are no-brainers, like adding missing spaces, as in 'a=3' to 'a = 3' or 'f (a,b = 3)' to 'f(a, b=3)', which also removes extra spaces. Rietveld's within-lines diffs make these easy to check. Other fixes are riskier. |
|
I would like to provide a final review before of any proposed changes. Also, along the way, I would happy to provide suggestions for more substantive changes (instead of shallow PEP-8 or PyLint changes). The primary defect in the modules is that the code got "adultified" somewhere along the way and needs to migrate back to the sort of straight-forward code that kids can read and model their code after. I've use this code to help train adults to teach kids and found that it needs to use a simpler feature set. |
|
I think the *some* of the 'adultification' that you refer to is a result of Gregor reimplementing turtle in a tkinter-independent intermediate language or two, the lowest layer of which he then implemented in tkinter as one particular backend. He intended to implement that next-to-lowest layer with other backends for use outside the stdlib. But unless we were to replace tkinter, that flexibility is just added and unneeded complexity for turtle in the stdlib. I originally read turtle.py to learn how to program the tkinter canvas. I hoped to see how visible turtle actions, especially animations, translated to canvas calls. I just downloaded 2.5.6 turtle.py (26kb instead of 145kb). At first glance, it seems more suited to that particular need. You would find it a better example of 'straight-forward code' for your classes. I just ran it from 2.7.6 Idle and the end-of-file demo runs. I have no knowledge of why the 2.5 turtle was replaced instead of being patched and upgraded. At the time, I was just a casual Python user who had never used turtle. |
|
Thank you so much for your support, Terry. I really appreciate all your feedback. Your comments have been very helpful as I am learning Turtle and Tkinter. Most of my changes are related to spacing, visual indentation, consistent line spaces between method definitions, and making sure all the code falls within 80 lines. They have been all been minor changes that wouldn't change the logic of the code. I haven't gotten to removing the commented out code yet. I will wait to commit this till I fix a bug in Turtle. |
|
A side note about tests: once a couple or three years ago I made it so that you could run 'make doctest' against the turtle documentation and it would work (displaying stuff on the screen). There's no checking of the output, but it proved the examples at least executed cleanly (with the help of some sphinx-hidden extra code). For whatever it is worth, they still appear to run to completion: Document: library/turtle |
|
In private email, Gregor indicated that he would like to be involved in turtle maintenance, and will be available about Aug. 1. |
|
Current status:
In fact, we should do this for the whole library, no special treatment is required for
Only three occurrences are left: Line 105 in e5b8b19 Lines 143 to 144 in e5b8b19 Lines 601 to 602 in e5b8b19
Only two occurrences are left: Lines 3754 to 3755 in e5b8b19 Lines 3756 to 3758 in e5b8b19 However, should we address these cosmetic issues? |
|
Reformatting correct, comprehensible, and needed code is against PEP 8 unless part of a bigger review and refactoring. Code is needed if is removal, as opposed to replacement, breaks some intended function. Removing unneeded code, whether discovered by a linter or otherwise, is routine as it inhibits performance or comprehension. So such removals are not purely cosmetic. I think all 3 commented-out chunks should be removed. I will prepare a PR with explanations. I will consider the XXX comments as I do so. |
arhadthedev
removed
the
pending
|
Except for these 5 items, I want to close this as I consider anything else as either done, obsolete, or something for another issue. |
* Remove the unused, private, and undocumented name `_ver` and the commented-out `print` call. * Don't add math functions to `__all__`. Beginners should learn to `import math` to access them. * Gregor Lindel, who wrote this version of turtle, dropped plans to implement turtle on another toolkit at least a decade ago. Drop `_dot` code preparing for this, but add a hint comment. * `_Screen` is meant to be a singleton class. To enforce that, it needs either a `__new__` that returns the singleton or `else...raise` in `__iter__`. Merely removing the `if` clauses as suggested might break something if a user were to call `_Screen` directly. Leave the code alone until a problem is evident. * Turtledemo injects into _Screen both _root and _canvas, configured as it needs them to be. Making _canvas an `__init__` option would require skipping some but not all of the lines under 'if _Screen._canvas is None:`. Leave working code alone.
* Remove the unused, private, and undocumented name `_ver` and the commented-out `print` call. * Don't add math functions to `__all__`. Beginners should learn to `import math` to access them. * Gregor Lindel, who wrote this version of turtle, dropped plans to implement turtle on another toolkit at least a decade ago. Drop `_dot` code preparing for this, but add a hint comment. * `_Screen` is meant to be a singleton class. To enforce that, it needs either a `__new__` that returns the singleton or `else...raise` in `__iter__`. Merely removing the `if` clauses as suggested might break something if a user were to call `_Screen` directly. Leave the code alone until a problem is evident. * Turtledemo injects into _Screen both _root and _canvas, configured as it needs them to be. Making _canvas an `__init__` option would require skipping some but not all of the lines under 'if _Screen._canvas is None:`. Leave working code alone.
* Remove the unused, private, and undocumented name `_ver` and the commented-out `print` call. * Don't add math functions to `__all__`. Beginners should learn to `import math` to access them. * Gregor Lindel, who wrote this version of turtle, dropped plans to implement turtle on another toolkit at least a decade ago. Drop `_dot` code preparing for this, but add a hint comment. * `_Screen` is meant to be a singleton class. To enforce that, it needs either a `__new__` that returns the singleton or `else...raise` in `__iter__`. Merely removing the `if` clauses as suggested might break something if a user were to call `_Screen` directly. Leave the code alone until a problem is evident. * Turtledemo injects into _Screen both _root and _canvas, configured as it needs them to be. Making _canvas an `__init__` option would require skipping some but not all of the lines under 'if _Screen._canvas is None:`. Leave working code alone.
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
Linked PRs
The text was updated successfully, but these errors were encountered: