Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Test cleanup #116

Closed
wants to merge 10 commits into from
Closed

Test cleanup #116

wants to merge 10 commits into from

Conversation

timdiller
Copy link
Contributor

I needed something completely brainless to do, so this PR introduces PEP8 compliance for the unit tests in traits/tests. It is almost entirely whitespace changes. There are a couple of separate commits that remove unused imports.

@sjagoe
Copy link
Contributor

sjagoe commented Jan 9, 2014

👍 on PEP8 compliance

@@ -18,12 +16,6 @@
# available online at http://www.enthought.com/licenses/BSD.txt
#
# Thanks for using Enthought open source!
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should have been removed?

@mdickinson
Copy link
Member

Not to be too grinchy about this, but this kind of wholesale change can mess up other development efforts, e.g., by breaking the mergeability of existing pull requests. I'd suggest checking the impact of this on the Python 3 PR before merging.


class Team ( HasTraits ):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is tricky: there's an obviously deliberate style choice on Dave Morrill's part to include spaces around function arguments and base classes, and much of the traits code follows that style. I think we should aim for local consistency over PEP8 consistency.

There's always the option of just fixing the whole codebase, but see my other comment...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would like to see traits move to PEP8 compliance (if just to shut my editor up...), but you make a good point. This branch should probably be based off the Python 3 effort as that is the most likely to have many conflicts.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FWIW, the use of spaces around function arguments is not strictly consistent applied, or at least it wasn't in the tests I looked at. Use of spaces seems to have varied from time to time.
Obviously, I feel we should move toward PEP8 compliance everywhere (otherwise I wouldn't have spent the time to do it). I have PEP8 and PyFlakes hilighting in my editor by default, and it makes looking at code like Traits painful, and cleaning it up is relatively easy. I also find I navigate PEP8 code more easily.
That said, I'm willing to be overruled if there is strong opinion to favor local consistency over an Enthought standard.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the technical concern of merge conflict is dispositive here. The Python 3 branch is almost ready to go in. We can do PEP8 cleanup afterwards.

@mdickinson mdickinson mentioned this pull request Jan 10, 2014
@pberkes pberkes closed this Jan 10, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants