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

pep8 for backend_pdf.py #2459

Merged
merged 4 commits into from Nov 24, 2013
Merged

Conversation

megies
Copy link
Contributor

@megies megies commented Sep 24, 2013

Pep8 cleanup for pdf backend.

@mdboom
Copy link
Member

mdboom commented Sep 30, 2013

@megies: Would you mind rebasing?

@megies
Copy link
Contributor Author

megies commented Sep 30, 2013

Uhm, I never did that before (in this scenario, just for not-yet-pushed/local stuff) and usually just merge. You want to avoid a merge commit in here, I guess? I would have to force push and rewrite my fork's history then after rebasing, right? Fine with that, just want to make sure..

@mdboom
Copy link
Member

mdboom commented Sep 30, 2013

That's correct. IMHO, a rebase is one of the few cases where a force push is ok.

@megies
Copy link
Contributor Author

megies commented Sep 30, 2013

Ok, I usually don't rewrite published history, but it won't really hurt here of course..

@tacaswell
Copy link
Member

Re-triggered travis. The last time this was run the pep8 checks were turned off on master. It is now failing because backend_pdf.py passes, but is on the exclude list.

Can you remove it from the exclude list in tests/test_coding_standards.py?

@megies
Copy link
Contributor Author

megies commented Oct 31, 2013

@tacaswell.. thanks for looking at the build. Done.

@tacaswell
Copy link
Member

It seems to have picked up conflicts and won't merge cleanly anymore, can you re-base again? 90% sure the conflict is another file being removed.

@megies
Copy link
Contributor Author

megies commented Oct 31, 2013

Sure, no problem. Actually the conflict comes from one of my pull requests (cleaning up the pep8 code checker structure to be reusable).. ;)

'Embedding TeX font ' + texname + ' - fontinfo=' + repr(fontinfo.__dict__),
'debug')
msg = 'Embedding TeX font ' + texname + ' - fontinfo=' + \
repr(fontinfo.__dict__)
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 we are trying to move away from \ line continuation to (...) continuation. iirc that is one of those things that they claim will be deprecated eventually (just like % formatting).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hmm, yes. Actually I also use ( ) normally, nowadays. I'll change it.

Copy link
Member

Choose a reason for hiding this comment

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

OT: Just to quickly note, I think it was decided that '' can never be
deprecated until a solution is found for line continuations for "with"
clauses (you can't use parens for multiple "with" targets). And they can
try to pull '%' formatting from my cold, dead hands!

Copy link
Member

Choose a reason for hiding this comment

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

On 2013/10/31 5:20 AM, Thomas A Caswell wrote:

In lib/matplotlib/backends/backend_pdf.py:

     fontdictObject = self.reserveObject('font dictionary')
     self.writeObject(fontdictObject, fontdict)
     return fontdictObject

 def embedTeXFont(self, texname, fontinfo):
  •    matplotlib.verbose.report(
    
  •        'Embedding TeX font ' + texname + ' - fontinfo=' + repr(fontinfo.**dict**),
    
  •        'debug')
    
  •    msg = 'Embedding TeX font ' + texname + ' - fontinfo=' + \
    
  •          repr(fontinfo.**dict**)
    

I think we are trying to move away from || line continuation to |(...)|
continuation. iirc that is one of those things that they claim will be
deprecated eventually (just like |%| formatting).

Use of the backslash has been contrary to mpl coding policy for a long
time, and is not related to any potential long-term shift to a new
continuation string--and as far as I know, there is no such new string.
One can use parentheses to avoid the trailing backslash. In a case like
the above, one can also make a list of the space-separated text chunks
to be concatenated and then use ' '.join(chunks).

@tacaswell
Copy link
Member

Left a bunch of picky comments, most of which I think you can ignore if you don't like them.

@megies
Copy link
Contributor Author

megies commented Oct 31, 2013

Both of the deletes you mentioned were unused variables if I remember correctly. But I can verify again..

@megies
Copy link
Contributor Author

megies commented Oct 31, 2013

Like mentioned those two deletes above were unused variables. Personally I would leave them out, but if anybody wants them left in there I'll add them again.

@tacaswell
Copy link
Member

👍 LGTM to me.

tacaswell added a commit that referenced this pull request Nov 24, 2013
@tacaswell tacaswell merged commit 5485cb9 into matplotlib:master Nov 24, 2013
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