PEP8 fixes on text.py #1357

Merged
merged 2 commits into from Nov 19, 2012

2 participants

@NelleV

PEP8 fixes on the module text

@dmcdougall dmcdougall and 1 other commented on an outdated diff Oct 10, 2012
lib/matplotlib/text.py
@@ -16,18 +16,15 @@
from matplotlib.patches import bbox_artist, YAArrow, FancyBboxPatch, \
FancyArrowPatch, Rectangle
import matplotlib.transforms as mtransforms
-from matplotlib.transforms import Affine2D, Bbox, Transform ,\
- BboxBase, BboxTransformTo
+from matplotlib.transforms import Affine2D, Bbox, Transform,\
@dmcdougall
Matplotlib Developers member
dmcdougall added a line comment Oct 10, 2012

There's a trailing backslash been left in at the end of this line.

@NelleV
NelleV added a line comment Oct 10, 2012

It's because the import takes two lines.

@dmcdougall
Matplotlib Developers member
dmcdougall added a line comment Oct 10, 2012

In another PR (I can't remember which one) you just added an extra from matplotlib.blah import statement at the beginning of the wrapped line. There was also a PR that used ( and ) to encase the imported items so they would wrap well. Either of those approaches trumps this one, I think.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@NelleV NelleV commented on the diff Oct 10, 2012
lib/matplotlib/text.py
# these are not available for the object inspector until after the
# class is build so we define an initial set here for the init
# function and they will be overridden after object defn
-docstring.interpd.update(Text = """
@NelleV
NelleV added a line comment Oct 10, 2012

I should probably work on making the documentation prettier, a bit more...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dmcdougall dmcdougall commented on the diff Oct 10, 2012
lib/matplotlib/text.py
@@ -839,7 +853,8 @@ def set_family(self, fontname):
the specific font names will be looked up in the
:file:`matplotlibrc` file.
- ACCEPTS: [ FONTNAME | 'serif' | 'sans-serif' | 'cursive' | 'fantasy' | 'monospace' ]
+ ACCEPTS: [FONTNAME | 'serif' | 'sans-serif' | 'cursive' | 'fantasy' |
+ 'monospace' ]
@dmcdougall
Matplotlib Developers member
dmcdougall added a line comment Oct 10, 2012

Here this list was broken after the | binary operator. On line 70 it was broken before.

@NelleV
NelleV added a line comment Oct 10, 2012

I think it is best to break after the |. I'll change l. 70 to be consistent.

@dmcdougall
Matplotlib Developers member
dmcdougall added a line comment Oct 10, 2012

Sure. To my eye I'm not fussed. I'm sure there will be fodder from others though. Thanks for being so patient and cooperative with us.

@NelleV
NelleV added a line comment Oct 10, 2012

Well, thanks for doing such boring reviews :)
Pep8 patches are always long tedious.

@dmcdougall
Matplotlib Developers member
dmcdougall added a line comment Oct 10, 2012

They are, and you've had to make decisions about where to break things and what to modify. When opinions start getting passed around involving those decisions it can lead to frustrations and sometimes upset. It's good to see you're taking none of it personally.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dmcdougall dmcdougall commented on the diff Oct 10, 2012
lib/matplotlib/text.py
@@ -880,7 +895,8 @@ def set_size(self, fontsize):
Set the font size. May be either a size string, relative to
the default font size, or an absolute font size in points.
- ACCEPTS: [ size in points | 'xx-small' | 'x-small' | 'small' | 'medium' | 'large' | 'x-large' | 'xx-large' ]
+ ACCEPTS: [size in points | 'xx-small' | 'x-small' | 'small' |
+ 'medium' | 'large' | 'x-large' | 'xx-large' ]
@dmcdougall
Matplotlib Developers member
dmcdougall added a line comment Oct 10, 2012

And here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dmcdougall dmcdougall commented on the diff Oct 10, 2012
lib/matplotlib/text.py
@@ -892,7 +908,10 @@ def set_weight(self, weight):
"""
Set the font weight.
- ACCEPTS: [ a numeric value in range 0-1000 | 'ultralight' | 'light' | 'normal' | 'regular' | 'book' | 'medium' | 'roman' | 'semibold' | 'demibold' | 'demi' | 'bold' | 'heavy' | 'extra bold' | 'black' ]
+ ACCEPTS: [a numeric value in range 0-1000 | 'ultralight' | 'light' |
+ 'normal' | 'regular' | 'book' | 'medium' | 'roman' |
+ 'semibold' | 'demibold' | 'demi' | 'bold' | 'heavy' |
+ 'extra bold' | 'black' ]
@dmcdougall
Matplotlib Developers member
dmcdougall added a line comment Oct 10, 2012

Ditto.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dmcdougall dmcdougall commented on the diff Oct 10, 2012
lib/matplotlib/text.py
@@ -904,7 +923,10 @@ def set_stretch(self, stretch):
"""
Set the font stretch (horizontal condensation or expansion).
- ACCEPTS: [ a numeric value in range 0-1000 | 'ultra-condensed' | 'extra-condensed' | 'condensed' | 'semi-condensed' | 'normal' | 'semi-expanded' | 'expanded' | 'extra-expanded' | 'ultra-expanded' ]
+ ACCEPTS: [a numeric value in range 0-1000 | 'ultra-condensed' |
+ 'extra-condensed' | 'condensed' | 'semi-condensed' |
+ 'normal' | 'semi-expanded' | 'expanded' | 'extra-expanded' |
+ 'ultra-expanded' ]
@dmcdougall
Matplotlib Developers member
dmcdougall added a line comment Oct 10, 2012

And again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@dmcdougall dmcdougall commented on the diff Oct 10, 2012
lib/matplotlib/text.py
@@ -1508,7 +1529,8 @@ def _get_xy_transform(self, renderer, s):
bbox0 = self.axes.bbox
# elif bbox_name == "bbox":
# if bbox is None:
- # raise RuntimeError("bbox is specified as a coordinate but never set")
+ # raise RuntimeError("bbox is specified as a coordinate but "
+ # "never set")
# bbox0 = self._get_bbox(renderer, bbox)
@dmcdougall
Matplotlib Developers member
dmcdougall added a line comment Oct 10, 2012

I wonder why this comment was left in...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@efiring efiring and 1 other commented on an outdated diff Oct 10, 2012
lib/matplotlib/text.py
@@ -1549,21 +1571,21 @@ def _get_ref_xy(self, renderer):
if isinstance(self.xycoords, tuple):
s1, s2 = self.xycoords
- if (is_string_like(s1) and s1.split()[0] == "offset") \
- or (is_string_like(s2) and s2.split()[0] == "offset"):
+ if (is_string_like(s1) and s1.split()[0] == "offset") or \
@efiring
Matplotlib Developers member
efiring added a line comment Oct 10, 2012

Here and below, I would prefer using parentheses rather than the trailing backslash.

@NelleV
NelleV added a line comment Oct 13, 2012

Fixed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@efiring efiring and 1 other commented on an outdated diff Oct 10, 2012
lib/matplotlib/text.py
@@ -111,20 +114,18 @@ def _get_textbox(text, renderer):
projected_xs = []
projected_ys = []
- theta = text.get_rotation()/180.*math.pi
- tr = mtransforms.Affine2D().rotate(-theta)
+ theta = text.get_rotation() / 180. * math.pi
+ tr = mtransforms.Affine2D().rotate(- theta)
@efiring
Matplotlib Developers member
efiring added a line comment Oct 10, 2012

Adding the space after the unary minus looks a bit odd, doesn't it?

@NelleV
NelleV added a line comment Oct 13, 2012

Yeah... I don't know why I did that.

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

@NelleV Can you squash down to one commit?

@WeatherGod WeatherGod commented on an outdated diff Oct 13, 2012
lib/matplotlib/text.py
def _update_position_xytext(self, renderer, xy_pixel):
- "Update the pixel positions of the annotation text and the arrow patch."
+ """Update the pixel positions of the annotation text and the arrow
+ patch."""
@WeatherGod
Matplotlib Developers member
WeatherGod added a line comment Oct 13, 2012

Multi-line docstrings should have the triple-quotes on their own lines.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@WeatherGod WeatherGod commented on the diff Oct 13, 2012
lib/matplotlib/text.py
@@ -1892,10 +1915,11 @@ def _update_position_xytext(self, renderer, xy_pixel):
if self._bbox_patch:
self.arrow_patch.set_patchA(self._bbox_patch)
else:
- patchA = d.pop("patchA", None)
@WeatherGod
Matplotlib Developers member
WeatherGod added a line comment Oct 13, 2012

Any clue what this was for? Could there still be code elsewhere that might need a "patchA" key popped out?

@NelleV
NelleV added a line comment Oct 13, 2012

I don't see where it would be needed, reading the code.

@WeatherGod
Matplotlib Developers member
WeatherGod added a line comment Oct 13, 2012
@NelleV
NelleV added a line comment Oct 14, 2012

So should I put it back ?

@efiring
Matplotlib Developers member
efiring added a line comment Oct 14, 2012

No, I would say you can leave it out, and we should close #1386. Note that the d.pop("patchA", None) is inside a block that is executed only if "patchA" is not in d, so it will never pop anything from d; all it does is to set "patchA = None", and patchA is not used from that point onward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@WeatherGod WeatherGod commented on an outdated diff Oct 13, 2012
lib/matplotlib/text.py
dsu.sort()
_, y = dsu[0]
shrink = d.pop('shrink', 0.0)
- theta = math.atan2(y-y0, x-x0)
- r = math.sqrt((y-y0)**2. + (x-x0)**2.)
- dx = shrink*r*math.cos(theta)
- dy = shrink*r*math.sin(theta)
+ theta = math.atan2(y - y0, x - x0)
+ r = math.sqrt((y - y0) ** 2. + (x - x0) ** 2.)
@WeatherGod
Matplotlib Developers member
WeatherGod added a line comment Oct 13, 2012

another place to use math.hypot()

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@WeatherGod WeatherGod and 3 others commented on an outdated diff Oct 13, 2012
lib/matplotlib/text.py
@@ -1966,15 +1991,13 @@ def update_bbox_position_size(self, renderer):
x_box, y_box, w_box, h_box = _get_textbox(self, renderer)
self._bbox_patch.set_bounds(0., 0.,
w_box, h_box)
- theta = self.get_rotation()/180.*math.pi
+ theta = self.get_rotation() / 180. * math.pi
@WeatherGod
Matplotlib Developers member
WeatherGod added a line comment Oct 13, 2012

Would this be more readable if we use np.deg2rad()?

@efiring
Matplotlib Developers member
efiring added a line comment Oct 13, 2012

or math.radians, since we know we are dealing with a scalar and don't need numpy's machinery.

@dmcdougall
Matplotlib Developers member
dmcdougall added a line comment Nov 11, 2012

Either is fine in my opinion, though I think I'd prefer just to stick with numpy.

Let's leave the decision up to @NelleV regarding which one to use. Once that change has been made I think this is fine to merge.

@NelleV
NelleV added a line comment Nov 13, 2012

I've changed all of those in np.deg2rad.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@efiring efiring commented on an outdated diff Oct 13, 2012
lib/matplotlib/text.py
- horizLayout[i] = thisx, thisy-(d + d_yoffset), \
+ horizLayout[i] = thisx, thisy - (d + d_yoffset), \
@efiring
Matplotlib Developers member
efiring added a line comment Oct 13, 2012

Instead of a trailing backslash, the RHS of this statement needs parentheses; it's a tuple.
Actually, I don't know why the line is broken at all; offhand, it looks like it would fit on a single line.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@efiring efiring and 1 other commented on an outdated diff Oct 13, 2012
lib/matplotlib/text.py
raise ValueError("xycoords should not be an offset coordinate")
x, y = self.xy
x1, y1 = self._get_xy(renderer, x, y, s1)
x2, y2 = self._get_xy(renderer, x, y, s2)
return x1, y2
- elif is_string_like(self.xycoords) and self.xycoords.split()[0] == "offset":
+ elif is_string_like(self.xycoords) and \
@efiring
Matplotlib Developers member
efiring added a line comment Oct 13, 2012

Please use parentheses here instead of trailing backslash.

@NelleV
NelleV added a line comment Oct 13, 2012

Fixed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@efiring efiring commented on an outdated diff Oct 13, 2012
lib/matplotlib/text.py
elif unit == "pixels":
tr = Affine2D()
elif unit == "fontsize":
fontsize = self.get_size()
dpi = self.figure.get_dpi()
- tr = Affine2D().scale(fontsize*dpi/72., fontsize*dpi/72.)
+ tr = Affine2D().scale(fontsize * dpi / 72.,
+ fontsize * dpi / 72.)
@efiring
Matplotlib Developers member
efiring added a line comment Oct 13, 2012

Minor suggestion, no insistence: since we have 4 instances of dpi / 72., and since it might not be completely obvious why, one could use a new variable, "dots_per_point" or "dpp" or "dppoint". I would probably use "dpp = dpi / 72.0" to keep it short and similar to "dpi". (And I like to include the zero after the decimal.)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@NelleV

I get a pyflakes warning on this file:

 text.py|26| W402 'TextPath' imported but unused 

Should I delete it ?

@dmcdougall
Matplotlib Developers member

I get a pyflakes warning on this file:

text.py|26| W402 'TextPath' imported but unused

Should I delete it ?

I'll open a separate issue for that once this is merged.

If nobody has any further objections, I'll merge this tomorrow.

@NelleV

I've rebased matplotlib/master on this branch: I think it is ready to be merged.

@dmcdougall dmcdougall merged commit c1b62af into matplotlib:master Nov 19, 2012

1 check passed

Details default The Travis build passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment