| 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) *  |
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) * |
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) * |
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) * |
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) * |
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. |
|
|