This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

Title: ipaddress unit tests PEP8
Type: enhancement Stage: resolved
Components: Tests Versions: Python 3.4, Python 3.5
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: exhuma, ncoghlan, pmoody, python-dev, r.david.murray, rhettinger
Priority: low Keywords: patch

Created on 2014-03-01 12:01 by exhuma, last changed 2022-04-11 14:57 by admin. This issue is now closed.

File name Uploaded Description Edit
test_ipaddress_pep8.patch exhuma, 2014-03-01 12:01 PEP8 fixes for Lib/test/ review
test_ipaddress_pep8-r3.patch exhuma, 2014-03-02 11:29 New patch which does not touch the old, imported tests. review
Messages (17)
msg212497 - (view) Author: Michel Albert (exhuma) * Date: 2014-03-01 12:01
While I was looking at the source of the ipaddress unit-tests, I noticed a couple of PEP8 violations. This patch fixes these (verified using the ``pep8`` tool).

There are no behavioural changes. Only white-space.

Test-cases ran successfully before, and after the change was made.
msg212500 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2014-03-01 13:00
Unfortunately, the pep8 tool includes some additional invented rules of its own, mostly related to being far too strict about indentation of continuation lines.

PEP 8 is actually mostly silent on that topic, merely pointing out a couple of specific things *not* to do. It's the tool that extrapolates a whole host of continuation line indentation rules that even PEP 8 itself doesn't meet.

There's also no blanket requirement in the PEP to have spaces around all binary operators (since that doesn't always aid readability), just a limited subset of them.

I've recently filed a bug against the tool itself, suggesting that given its name, its default behaviour should be limited to things that are actually backed up by the PEP.

That said, I think some of the cases picked up here do represent genuine readability improvements due to improved alignment. They just need to be separated out from the spurious changes resulting from invented rules in the pep8 tool.
msg212516 - (view) Author: Michel Albert (exhuma) * Date: 2014-03-01 17:07
Thanks for the quick reply!

I did not know the pep8 tool added it's own rules :( I have read PEP8 a long while ago and have since relied on the tool to do "the right thing". Many of it's idiosyncrasies have probably made their way into my blood since :(

And you're right: The PEP is actually explicitly saying that it's okay to leave out the white-space to make operator precedence more visible (for reference:

I will undo those changes.

Is there anything else that immediately caught your eye so I can address it in the update?
msg212528 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2014-03-01 23:40
I'd suggest focusing more on the structured tests at the top of the file - the ones further down we largely incorporated wholesale from the original ipaddr project, and didn't go back and enforce PEP 8 compliance, but the new tests should follow the style guide.
msg212547 - (view) Author: Michel Albert (exhuma) * Date: 2014-03-02 11:29
Here's a new patch which addresses white-space issues without touching the old tests.
msg212601 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2014-03-02 23:23
FWIW, I consider this patch to be a waste of time and don't want to encourage more patches like it.   Please focus on substantive changes like improving test coverage, adding documentation strings, etc.

Applying this patch would make a microscopic improvement to neatness but it also has costs (wasting Nick's time, masking the actual test developer's name in the Hg file annotation, and making it more difficult to apply cross-version bug fixes).

PEP-8 is a set of guidelines, not a rigid set of rules where "violations" must be sought out and eliminated.   That is not the spirit of the PEP.

I recommend closing this as won't fix.
msg212606 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2014-03-03 02:04
I think there's still value in giving someone experience with the current patch development process, as well as looking for potentially genuine readability improvements.

However, I also agree that automated compliance with a style guide checker can't sensibly be retrofitted to an existing project, even if that project is the originator of the style guide (it generates too much noise in the code history, and risks creating spurious merge conflicts for other changes. In the specific case of the standard library, we also sometimes make exceptions to the style guide to reduce code churn when merging in an existing project, as happened in the case of ipaddress).

In this case, I like the alignment fixes in the custom assertion methods, so I'll probably apply those as a small readability improvement.
msg212635 - (view) Author: Michel Albert (exhuma) * Date: 2014-03-03 13:18
I strongly agree with Raymond's points! They are all valid.

I should note, that I submitted this patch to - as mentioned by Nick - familiarise myself with the patch submission process. I decided to make harmless changes which won't risk braking anything.
msg213151 - (view) Author: pmoody (pmoody) * (Python committer) Date: 2014-03-11 16:59
Nick, did you want me to apply this?
msg213176 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2014-03-11 21:27
Yeah, I like the small readability tweaks in the r3 patch, so feel free to apply that one.
msg213270 - (view) Author: pmoody (pmoody) * (Python committer) Date: 2014-03-12 16:26
Will Do.

Michael, would you mind signing the contributor agreement so I can apply your patch?
msg213271 - (view) Author: Michel Albert (exhuma) * Date: 2014-03-12 16:28
Did so already last weekend. I suppose it will take some time to be processed.

I can ping you via a message here once I receive the confirmation.
msg213272 - (view) Author: pmoody (pmoody) * (Python committer) Date: 2014-03-12 16:40
msg214560 - (view) Author: Michel Albert (exhuma) * Date: 2014-03-23 10:51
It seems the contributor agreement form has been processed. As I understand it, the asterisk on my name confirms this.

I also verified that this patch cleanly applies to the most recent revision.
msg218236 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2014-05-10 20:20
Marking as low priority.  This patch really isn't worth applying.

If someone wants to starting contributing, there are plenty of useful things to do (such as adding docstrings to elementtree).  I don't want to encourage non-substantive shallow code churn and eats developer time for no benefit.

From my point of view, the "benefit" is actually negative because these sort of micro-rearrangements clutter the "annotate" history and because it makes it more difficult to apply substantive patches across multiple versions of Python.
msg229192 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2014-10-12 19:17
New changeset 27a02eb9273f by R David Murray in branch '3.4':
#20815: small readability improvements in ipaddress tests.

New changeset 4c9d27eef892 by R David Murray in branch 'default':
#20815: small readability improvements in ipaddress tests.
msg229193 - (view) Author: R. David Murray (r.david.murray) * (Python committer) Date: 2014-10-12 19:19
Since ipaddress is relatively new, the package maintainers approved the patch, and it is mostly blank lines (and therefore not a problem with
annotate) I committed this.  Thank Michel.
Date User Action Args
2022-04-11 14:57:59adminsetgithub: 65014
2014-10-12 19:19:13r.david.murraysetstatus: open -> closed

versions: + Python 3.4
nosy: + r.david.murray

messages: + msg229193
resolution: fixed
stage: commit review -> resolved
2014-10-12 19:17:53python-devsetnosy: + python-dev
messages: + msg229192
2014-05-10 20:20:00rhettingersetpriority: normal -> low

messages: + msg218236
2014-05-09 12:45:58ezio.melottisettype: enhancement
stage: commit review
2014-03-23 10:51:42exhumasetmessages: + msg214560
2014-03-12 16:40:17pmoodysetmessages: + msg213272
2014-03-12 16:28:06exhumasetmessages: + msg213271
2014-03-12 16:26:37pmoodysetmessages: + msg213270
2014-03-11 21:27:54ncoghlansetmessages: + msg213176
2014-03-11 16:59:44pmoodysetmessages: + msg213151
2014-03-03 13:18:17exhumasetmessages: + msg212635
2014-03-03 02:04:44ncoghlansetmessages: + msg212606
2014-03-02 23:23:36rhettingersetnosy: + rhettinger
messages: + msg212601
2014-03-02 11:29:48exhumasetfiles: + test_ipaddress_pep8-r3.patch

messages: + msg212547
2014-03-01 23:40:49ncoghlansetmessages: + msg212528
2014-03-01 17:07:08exhumasetmessages: + msg212516
2014-03-01 13:00:07ncoghlansetmessages: + msg212500
2014-03-01 12:01:32exhumacreate