Pep8 on many tests #2059
Pep8 on many tests #2059
Conversation
| @@ -93,16 +89,6 @@ def test_collection_transform_of_none(): | |||
| assert isinstance(c._transOffset, mtrans.IdentityTransform) | |||
|
|
|||
|
|
|||
| def test_point_in_path(): | |||
There was a problem hiding this comment.
Great. Over time, I'd like to move a lot of stuff out of test_axes as well so that no one file so dominates the run time.
|
@mdboom - if you happy, lets merge this and we can continue to keep up some momentum on PEP8 improvements. |
| [58230, 381139, 78045, 99308, 160454], | ||
| [89135, 80552, 152558, 497981, 603535], | ||
| [78415, 81858, 150656, 193263, 69638], | ||
| [139361, 331509, 343164, 781380, 52269]] |
There was a problem hiding this comment.
Seems like a downgrade. Any way to ignore this kind of whitespace checking in the tool so we don't have to do things like this?
There was a problem hiding this comment.
I'm not sure. Each and every exception and line can be intercepted, and there may be a way to do inline markup to skip the checking, but in truth I don't necessarily agree that this is a downgrade. If you feel strongly about this we could pad the numbers with zeros?
There was a problem hiding this comment.
I wasn't advocating inline markup -- I think that that would certainly be a downgrade. I was wondering if it would be possible to turn this whitespace check off globally since I generally think that extra whitespace before a literal or token like this can improve readability. Unfortunately, it doesn't look like one can do that effectively. The pep8 tool parses this as spaces after commas (and [ and whatever else), not spaces before literals, so it can't easily be turned off without letting through a bunch of other genuine problems. sigh. Maybe with some preprocessing this could be achieved. I just hate to throw out good violations of PEP8 because we want to eliminate bad violations elsewhere.
There was a problem hiding this comment.
@mdboom, I agree entirely, and I think we need to resist the absolute tyranny of mechanical PEP8 checking, which is not consistent with PEP8 itself, or with good practice. The point is to make code maximally readable by humans.
There was a problem hiding this comment.
I realize I'm being annoyingly late to this suggestion, having already merged the PEP8 test, but perhaps rather than failing on PEP8 violations we should just be tracking their total number. That's what we do on the astropy project, for example. If a particular large PR increases the number by a large amount, we address it, otherwise, there is always just some background of PEP8 violations we expect to have.
Another alternative might be to just turn off all whitespace-related checks in the automated checking, even though that will let some things through that are uncontroversially bad (such as function_call ( x ), for example. The other whitespace rule that I think is worthy of violating rather frequently is the grouping of operators for precedence, i.e. x + x**2. The fact that PEP8 doesn't encourage this is just a sign of its background in non-scientific programming, IMHO.
If this is becoming too much of a monster to get right, I propose we leave the pep8 test in, but comment out the assert statement that actually causes it to fail.
There was a problem hiding this comment.
I think ignoring E226 takes care of the operator group issues and that is one of the default ignores.
There was a problem hiding this comment.
For the sake of moving this PR forwards, I've reverted this change.
|
Sure -- I think this is mostly harmless. Some of this we may want to hold off on until after the feature freeze, though. |
|
In case its hidden by github I commented: |
I'm not sure I follow the logic here. Merging this into v1.3.x would mean that any future changes to these files will be done on top of this work - that will prevent any future conflicts, relating to PEP8, when merging |
|
The logic is that these changes are not strictly necessary (not really bug fixes), and they have the risk of introducing bugs. There have been other seemingly innocuous PEP8 changes that introduced bugs, and I would hate to inadvertently introduce more here this late in the game as well. |
|
@pelson I think is fine to merge once you give it a rebase. |
|
It would be nice to merge this. Can you try rebasing? |
|
Done. See 38e5eab. Tests on the conflicted files passed locally. |
|
Cool. Thanks @dmcdougall |
No description provided.