PEP8 on artist #1153

Merged
merged 0 commits into from Aug 29, 2012

3 participants

@NelleV

This is a simple PEP8 PR on the submodule artist I did while reading the code.

@pelson pelson and 1 other commented on an outdated diff Aug 27, 2012
lib/matplotlib/artist.py
@@ -753,15 +753,17 @@ def findobj(self, match=None, include_self=True):
.. plot:: mpl_examples/pylab_examples/findobj_demo.py
"""
- if match is None: # always return True
- def matchfunc(x): return True
+ if match is None: # always return True
@pelson
Matplotlib Developers member
pelson added a line comment Aug 27, 2012

The comment is in the wrong place (not your fault) and is therefore misleading. Should be on the function itself.

@NelleV
NelleV added a line comment Aug 28, 2012

I actually think there is a problem with this method. Not far after this code, there an::

 elif callable(match):
    matchfunc = func

func is not defined anywhere in this method. I'm pretty sure it is meant to be matchfunc = match.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pelson pelson and 2 others commented on an outdated diff Aug 27, 2012
lib/matplotlib/artist.py
@@ -864,11 +867,14 @@ def _get_setters_and_targets(self):
setters = []
for name in dir(self.o):
- if not name.startswith('set_'): continue
+ if not name.startswith('set_'):
+ continue
o = getattr(self.o, name)
@pelson
Matplotlib Developers member
pelson added a line comment Aug 27, 2012

Wow. o is perhaps the worst variable name I can think of (although l is right up there too). Not your fault, and not something you should address in this PR. Just wanted to draw some attention to it.

@dmcdougall
Matplotlib Developers member
dmcdougall added a line comment Aug 28, 2012

I don't know, I've belted out classics such as asd, asdasd, asdasdasd, roflcopter, rofl and copter in my PhD code. It's not (yet) publicly available.

People are going to hate me.

@pelson
Matplotlib Developers member
pelson added a line comment Aug 29, 2012

:-)

@NelleV
NelleV added a line comment Aug 29, 2012

I'm OK fixing that, but I'm not sure what o is yet. I'll do it when I have a better understanding of this code.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pelson pelson and 2 others commented on an outdated diff Aug 27, 2012
lib/matplotlib/artist.py
@@ -905,11 +912,12 @@ def aliased_name(self, s):
"""
if s in self.aliasd:
- return s + ''.join([' or %s' % x for x in self.aliasd[s].iterkeys()])
+ return s + ''.join([' or %s' % x
@pelson
Matplotlib Developers member
pelson added a line comment Aug 27, 2012

Its times like these that I personally would not follow the PEP8 guidelines.

@NelleV
NelleV added a line comment Aug 28, 2012

IMO, it is better than more than 80 char lines :)

There's probably a best way to write this code.

@efiring
Matplotlib Developers member
efiring added a line comment Aug 28, 2012

E.g., break it into two steps, starting with "aliases = self.aliasd[s].iterkeys()".

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pelson
Matplotlib Developers member

Looks good. For me this is a massive improvement, despite the fact that I don't really like wrapping list comprehensions.

Will merge this in 48 hours.

@mdboom
Matplotlib Developers member

Please don't merge this until the branch has been created. We're in a feature freeze.

@pelson
Matplotlib Developers member

We're in a feature freeze.

This doesn't add any features.

Merging this after taking the 1.2.x branch will cause horrible conflicts (either for the merger of this, or for whoever took branches before this got merged). We have restrained from implementing PEP8 changes during the v1.1.x lifecycle for precisely these reasons and IMHO now is the perfect time to do this kind of leg work (for which we are all grateful @NelleV ).

I will not merge while @mdboom holds the objection, but I cannot see a better time to merge it than right before a new release branch is taken.

@mdboom
Matplotlib Developers member

My concern is about keeping the existing PRs for 1.2.x to apply cleanly without requiring a potentially time consuming rebase. It's less of a concern to have difficult to merge things later.

@NelleV

I can keep the PR up to date with merges in master until you think it is OK to be merged if you don't mind me rebasing the code.

@mdboom
Matplotlib Developers member

Let's do this as a compromise -- I'm going to try to clean out as many other PRs for 1.2.x today as I can, and then following that we can put some cleanup things (such as this) prior to the RC.

@NelleV

I'm not familiar with matplotlib's development process. I'm cleaning up some other files. Should I update this PR with the other PEP8 compliant file or keep that "in stock" until you guys are ready to merge the PEP8 patches ?

@mdboom
Matplotlib Developers member

Feel free to add them to this PR or a new PR. (Doesn't really matter). The only thing we're holding off on a bit is the merging.

Thanks for this work, by the way. It's long overdue.

@NelleV NelleV and 1 other commented on an outdated diff Aug 28, 2012
lib/matplotlib/colors.py
self._isinit = False
-
+ # FIXME FIXME FIXME bytes is a *keyword* in python
@NelleV
NelleV added a line comment Aug 28, 2012

I'm not sure if we should deal with this small issue here: bytes is a keyword in python. Changing that would, I guess, break backwards compatibility.

@efiring
Matplotlib Developers member
efiring added a line comment Aug 29, 2012

That should be a separate PR, which can involve introducing a new keyword ("asbytes"?) and deprecating the old one with a warning, while changing all uses within mpl to the new version. Not urgent.

(bytes is not a python keyword but a relatively recent builtin type.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@pelson
Matplotlib Developers member

@NelleV: I think it would be better to remove those last 2 commits. In general, more smaller PRs are better than fewer larger ones. I would be happy to review each file separately (given that these are considerable in size, if not in code changes). Thanks.

@NelleV

OK, I'll create severals PRs. If that's OK, I'd like to close this one, and create a new one in a branch. I didn't expect the merge to be delayed so I worked in master. It's now being a burden for me.

@pelson
Matplotlib Developers member

Sure. You can push a renamed branch to your fork without a problem (keeping the same commit sha):

git push origin 72f6b07f38ea4f312b8869b62e05cffad63bb3b3:figure_pep8

might just do the trick.

@pelson
Matplotlib Developers member

For the other two, you can just take new branches and cherry pick the respective commits.

@NelleV NelleV merged commit 0c7cdaa into matplotlib:master Aug 29, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment