classification
Title: Cleaned source of `cmd` module
Type: enhancement Stage: resolved
Components: Library (Lib) Versions: Python 3.4
process
Status: closed Resolution: not a bug
Dependencies: Superseder:
Assigned To: Nosy List: amaury.forgeotdarc, eric.araujo, r.david.murray, terry.reedy, zearin
Priority: normal Keywords:

Created on 2012-06-22 14:02 by zearin, last changed 2012-06-24 19:36 by eric.araujo. This issue is now closed.

Files
File name Uploaded Description Edit
cmd.py zearin, 2012-06-22 14:02
patch-cmd.txt zearin, 2012-06-22 14:22 Patch between my edits and ace45d23628a
Messages (7)
msg163409 - (view) Author: Zearin (zearin) Date: 2012-06-22 14:02
Cleaned up the source of the Standard Library’s `cmd` module.  

Attempted to focus on readability by changing things like using booleans instead of 0/1, newer syntax for string formatting, lining up variable declarations (judgement call), and adding comments between groups of methods.

Used ace45d23628a to start my work.
msg163410 - (view) Author: Amaury Forgeot d'Arc (amaury.forgeotdarc) * (Python committer) Date: 2012-06-22 14:13
The output of "hg diff" would be much appreciated, it's the only way for reviewers to realize the changes you made.
msg163412 - (view) Author: Zearin (zearin) Date: 2012-06-22 14:22
Does `hg diff` produce different output than regular `diff`?  

If so, the patch created by regular `diff` is attached.

(I basically did my work without using a local repo, and I have very little experience with patching.  I basically just looked up how to create a patch with vanilla `diff`.)
msg163413 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2012-06-22 14:31
I appreciate the work and thought that went into this, but this is not normally the kind of change that we apply.  Any change has a chance of introducing bugs (see, for example, issue 15109, where a seemingly innocuous change from % formatting to .format formatting caused a regression in 2.7.3 compared to 2.7.2).

The other option, applying it just to default, is also not normally done, because then the code base between versions is gratuitously different, making merging more difficult.

So, our general policy is that we do cleanups only when we are changing that area of code in order to fix a bug or add an enhancement.  And as you can see from issue 15109, even then we sometimes get in trouble.

Now, if you are interested in the maintenance of the cmd module, and can suggest one or more fix/enhancement + cleanup patches like Lucasz Lagna did for configparser, that would be wonderful.
msg163414 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2012-06-22 14:36
Also as an FYI, triple double quote marks are our standard for docstrings.  I don't believe we normally align assignments, but I don't know if that is specifically called out in PEP8.  I haven't looked at the patch in detail, but in scanning it quickly, while some of your whitespace changes (spaces around operators) agree with our standards, others (extra spaces after a comma in a function call, for example) do not.

Again, I appreciate your putting in this effort, and regret that it isn't something we can use.
msg163551 - (view) Author: Terry J. Reedy (terry.reedy) * (Python committer) Date: 2012-06-23 08:40
Do read PEP 8 Python style guide. http://python.org/dev/peps/pep-0008/

You violated the following:
(Peeves)
More than one space around an assignment (or other) operator to align it with another.

Yes:

x = 1
y = 2
long_variable = 3

No:

x             = 1
y             = 2
long_variable = 3

I used to do that, but it only works with fixed-pitch fonts, which is not really possible for full-unicode fonts. Anyway, that is about half the changes, and they would have to go. Sorry. Some of your other changes make it more compliant. Some I am not sure of others without re-reading.

For the other reasons David gave, I am closing this so you are not mislead into doing more work that will not be accepted. I would note that improving test coverage *is* accepted and good test-coverage is really needed before extensive re-writes. Another document to read is
the developer guide http://docs.python.org/devguide/index.html

Last point. Please use .diff or .patch for diff/patch files as that extension works better for people and, I believe, hg.

Since you are interested in readability, you might consider contributing doc suggestions. You do not have to know .rst formatting. A good suggestion given as plain ascii in a message like this will be copied and formatted by someone who does know .rst. And in simple cases, one can even patch the source .rst withouth knowing much.
msg163825 - (view) Author: Éric Araujo (eric.araujo) * (Python committer) Date: 2012-06-24 19:28
Just wanted to offer a comment on this:
> Last point. Please use .diff or .patch for diff/patch files as that
> extension works better for people
Depending on OS and file association settings, yes.

> and, I believe, hg.
Mercurial does not care about file extensions, it works with standard input or any file path or URI.
History
Date User Action Args
2012-06-24 19:36:30eric.araujosetresolution: not a bug
stage: resolved
2012-06-24 19:28:35eric.araujosetstatus: open -> closed
2012-06-24 19:28:04eric.araujosetstatus: closed -> open

nosy: + eric.araujo
messages: + msg163825

resolution: rejected -> (no value)
stage: resolved -> (no value)
2012-06-23 08:40:47terry.reedysetstatus: open -> closed

versions: - Python 3.1, Python 2.7, Python 3.2, Python 3.3
nosy: + terry.reedy

messages: + msg163551
resolution: rejected
stage: resolved
2012-06-22 14:36:51r.david.murraysetmessages: + msg163414
2012-06-22 14:31:15r.david.murraysetnosy: + r.david.murray
messages: + msg163413
2012-06-22 14:22:37zearinsetfiles: + patch-cmd.txt

messages: + msg163412
2012-06-22 14:13:24amaury.forgeotdarcsetnosy: + amaury.forgeotdarc
messages: + msg163410
2012-06-22 14:02:27zearincreate