PEP8 fixes on the module texmanager #1550

Merged
merged 1 commit into from Dec 3, 2012

Conversation

Projects
None yet
3 participants
Contributor

NelleV commented Dec 3, 2012

Here is some pep8 fixes on the texmanager.

Cheers,
N

Contributor

NelleV commented Dec 3, 2012

There are things that are deprecated, or here because they don't work with python2.5, which isn't supported anymore. I could use this PR to remove them, or do it in another PR.

Member

dmcdougall commented Dec 3, 2012

@NelleV I say do it in another PR. I'll open a separate issue for it. Thanks for reporting it.

Member

dmcdougall commented Dec 3, 2012

See #1552.

Member

pelson commented Dec 3, 2012

👍 - @dmcdougall please merge when your happy.

Cheers @NelleV

Member

dmcdougall commented Dec 3, 2012

@pelson Thanks for checking this out. I'd also like to give it a once-over before pushing 'go'.

@dmcdougall dmcdougall and 1 other commented on an outdated diff Dec 3, 2012

lib/matplotlib/texmanager.py
@@ -386,16 +410,17 @@ def make_dvi_preview(self, tex, fontsize):
returns the file name
"""
basefile = self.get_basefile(tex, fontsize)
- dvifile = '%s.dvi'% basefile
- baselinefile = '%s.baseline'% basefile
+ dvifile = '%s.dvi' % basefile
+ baselinefile = '%s.baseline' % basefile
if DEBUG or not os.path.exists(dvifile) or \
@dmcdougall

dmcdougall Dec 3, 2012

Member

Trailing backslash.

@NelleV

NelleV Dec 3, 2012

Contributor

FIXed

@dmcdougall dmcdougall commented on the diff Dec 3, 2012

lib/matplotlib/texmanager.py
@@ -587,24 +626,23 @@ def get_text_width_height_descent(self, tex, fontsize, renderer=None):
if rcParams['text.latex.preview']:
# use preview.sty
basefile = self.get_basefile(tex, fontsize)
- baselinefile = '%s.baseline'% basefile
-
+ baselinefile = '%s.baseline' % basefile
if DEBUG or not os.path.exists(baselinefile):
dvifile = self.make_dvi_preview(tex, fontsize)
with open(baselinefile) as fh:
l = fh.read().split()
@dmcdougall

dmcdougall Dec 3, 2012

Member

Good lord. Horrible variable name.

@pelson

pelson Dec 3, 2012

Member

😄 - your loving this @dmcdougall! Personally, I'm quite happy to accept that @NelleV has made an improvement and not go through all of the remaining code, but I have to say your more thorough review will result in the better overall code changes.

@NelleV

NelleV Dec 3, 2012

Contributor

Do you want me to fix this variable name ? I'm not sure what it corresponds to, so I'm not sure what a good variable name is for it.

@dmcdougall

dmcdougall Dec 3, 2012

Member

@pelson I wasn't suggesting we go through and fix all the variable names... that is a gargantuan task (I'd like to have them all fixed in all the other files in the codebase too). Although it's better style to have variable names that are a little less, shall we say opaque, I am happy for this to be merged when the trailing backslash above is fixed.

@dmcdougall

dmcdougall Dec 3, 2012

Member

@NelleV No that's ok.

Member

dmcdougall commented Dec 3, 2012

Cheers @NelleV!

@dmcdougall dmcdougall added a commit that referenced this pull request Dec 3, 2012

@dmcdougall dmcdougall Merge pull request #1550 from NelleV/pep8_textmanager
PEP8 fixes on the module texmanager
156867e

@dmcdougall dmcdougall merged commit 156867e into matplotlib:master Dec 3, 2012

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