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

Fix PEP8 coding style violations #3924

Merged
merged 2 commits into from Dec 24, 2014
Merged

Fix PEP8 coding style violations #3924

merged 2 commits into from Dec 24, 2014

Conversation

habi
Copy link
Contributor

@habi habi commented Dec 16, 2014

pep8 *.py --ignore=E501 gives no output anymore.

TODO: Fix long lines

As part of http://24pullrequests.com

`pep8 *.py --ignore=E501` gives no output anymore.

TODO: Fix long lines

As part of http://24pullrequests.com
`pep8 unit/*.py --ignore=E501` tells us everything is ok.

As part of http://24pullrequests.com
@@ -1,4 +1,5 @@
import sys, datetime
import sys
import datetime

if len(sys.argv) != 3:
print >>sys.stderr, 'Usage: license.py version_num filename'
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, looks like license.py isn't py3k compatible...

@WeatherGod
Copy link
Member

I have completely forgotten about the stuff in "unit". I don't even know if they work anymore, and I don't think the tests in there even get invoked in any way. As far as this PR is concerned, I am fine with it, but we definitely need to figure out what to do with the stuff in "unit".

@jenshnielsen
Copy link
Member

tools/gh_api.py and tools/github_stats.py are copied from IPython and really shouldn't be modified more than we have to within matplotlib

@tacaswell
Copy link
Member

units still works, it is how we do dates. I started to dig into it a few weeks ago and it seem fine, if opaque. What it really needs is some (extensive) documentation.

👍 on not modifying files that are not ours.

@WeatherGod
Copy link
Member

I am not talking about lib/matplotlib/units.py. I am talking about unit/

On Tue, Dec 16, 2014 at 9:53 AM, Thomas A Caswell notifications@github.com
wrote:

units still works, it is how we do dates. I started to dig into it a few
weeks ago and it seem fine, if opaque. What it really needs is some
(extensive) documentation.

[image: 👍] on not modifying files that are not ours.


Reply to this email directly or view it on GitHub
#3924 (comment)
.

@tacaswell
Copy link
Member

Ah, those. Never mind.

Although, my statement abount documentation on units.py I think still stands.

@habi
Copy link
Contributor Author

habi commented Dec 16, 2014

@jenshnielsen I wasn't aware that tools/*.py is copied from IPython (should have read the docstring).
Please ignore them if necessary.
IPython doesn't seem to follow pep8 strictly: ipython/ipython#7228

@WeatherGod
Copy link
Member

Yeah, that guy and I got into a huge (somewhat druken) argument about PEP8 PRs back at SciPy2014... nobody won that argument...

@jenshnielsen
Copy link
Member

@habi no problem. Perhaps we should make that more obvious somehow.

@tacaswell
Copy link
Member

@habi Can you just remove the commit touching the tools folder ( 7497d1a) from the PR? A force push to your branch from the previous commit will do it I think.

@habi
Copy link
Contributor Author

habi commented Dec 23, 2014

@tacaswell I reverted the commit that touched the tools folder.
Is that good enough, or should I kill these commits from history?

@tacaswell
Copy link
Member

7e0fdb5 also looks kind of funny....

For neatness I would prefer to just get rid of those commits from the history.

@habi
Copy link
Contributor Author

habi commented Dec 24, 2014

I think I did, by git reset --hard e4dc869 and git push --force.

tacaswell added a commit that referenced this pull request Dec 24, 2014
PEP8 : Fix PEP8 coding style violations in unit and helper scripts
@tacaswell tacaswell merged commit 8301087 into matplotlib:master Dec 24, 2014
@tacaswell
Copy link
Member

Thanks.

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.

None yet

5 participants