Repository navigation
Cleanups - #9125
Merged
Merged
Cleanups#9125
Conversation
dopplershift
approved these changes
Aug 30, 2017
story645
reviewed
Aug 31, 2017
| if prev and prev.latex_header == latex_header and prev.texcommand == texcommand: | ||
| if rcParams.get("pgf.debug", False): | ||
| # Check if the previous instance of LatexManager can be reused. | ||
| if (prev and prev.latex_header == latex_header |
Member
There was a problem hiding this comment.
order of operations unclear here (and yes I know was unclear previously too), what about instead using parenthesis around each == pair?
Contributor
Author
There was a problem hiding this comment.
I think it is pretty well known that "and" and "or" have lower priority than comparison operators. Relying on this basically occurs everywhere throughout the codebase.
Member
There was a problem hiding this comment.
shrugs I never remember this stuff, but not gonna hold up the review over it.
Member
There was a problem hiding this comment.
explicit is better than implicit, but too many () can also make it confusing by adding to visual noise. In this case I think it is ok without ()
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
I noticed while working on #9124 that some old codepaths seem to assume keys don't necessarily exist in rcParams, which may possibly have been the case a long time ago but is not the case anymore today -- so remove all rcParams.get by indexing into rcParams.
... except in axis3d which tries to look up the nonexistent ztick.major.width and ztick.color, so keep the fallback on xtick.major.width and xtick.color (I did not add the whole suite of ztick.* rcParams to rcsetup because in that case we'd better make sure all of them are respected, which is another project in itself). Now that I was there I could as well do some style cleanups in axis3d.py.
PR Summary
PR Checklist