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

Add axes.titleweight as an rc param #2574

Merged
merged 4 commits into from Nov 14, 2013
Merged

Conversation

adrn
Copy link
Contributor

@adrn adrn commented Nov 4, 2013

This was mentioned in #372 a while back -- the comment was that all font objects should accept the same configuration options, but this hasn't happened yet. Here's another quick fix in the meantime...

cc @mdboom

@adrn
Copy link
Contributor Author

adrn commented Nov 4, 2013

Sheesh, strict pep8 compliance...79 chars is soooo 1928 :)

@tacaswell
Copy link
Member

Can you please fix the problem?

If you want to start a discussion about dropping that section of pep8 this is not the place to do it.

@adrn
Copy link
Contributor Author

adrn commented Nov 4, 2013

::sigh::

Done.

@@ -895,7 +895,8 @@ def cla(self):
self.containers = []

self.grid(self._gridOn, which=rcParams['axes.grid.which'])
props = font_manager.FontProperties(size=rcParams['axes.titlesize'])
props = font_manager.FontProperties(size=rcParams['axes.titlesize'],
weight=rcParams['axes.titleweight'])
Copy link
Member

Choose a reason for hiding this comment

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

Why doesn't this use set_title?

I think props and the bunch of code below it should be replaced with three calls to set_title.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, but I didn't modify that code -- are you suggesting a new, separate change?

Copy link
Member

Choose a reason for hiding this comment

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

I am suggesting extra changes as long as you are touching this section of the code.

It looks like not all of the rcparams are followed here (baseline) so you get to squash a bug while your at it.

Copy link
Member

Choose a reason for hiding this comment

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

To be fair, personally I'm comfortable @adrn not adding the baseline rcParam here - it will take more time to investigate that that is the correct behaviour, and we don't even have an end user who has highlighted this as a problem.

@tacaswell - having already invested some time in looking at the prospect of using set_title in the cla method, are you prepared to accept this change, and optionally have a follow on PR which addresses this existing bug?

Copy link
Member

Choose a reason for hiding this comment

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

On further investigation, I take my comments back. The __init__ function uses cla to create the title objects so they can't be rolled up without a whole bunch of other changes.

@pelson
Copy link
Member

pelson commented Nov 5, 2013

Every time I see the rcParameters being extended I feel like I need to get into a hot shower and scrub myself clean - particularly with the badly handled font type parameters such as the axes.title* ones.

Nonetheless, the change is sound and I'm not against it. @adrn - the only comment would be that a unit test would be good to have (in an ideal world, this would make use of mock/some-other-tool to isolate this change into a non-visual test).

Thanks for persevering with the PEP8-ed ness of our codebase @adrn - I know the 80 character limit sounds nuts, but anything else and you end up inconsistent with other Python packages, or a "fuzzy" type limit we would end up with gray areas. And on the plus side, at least you know what to expect when changes get reviewed as the PEP8 guidelines are ubiquitous 😉

@tacaswell
Copy link
Member

Every kwarg has a default value -> someone will want an rcparam for it. I wonder if some sub-set of these would be better handled via clever use of partial.

dmcdougall added a commit that referenced this pull request Nov 14, 2013
Add axes.titleweight as an rc param
@dmcdougall dmcdougall merged commit c424d17 into matplotlib:master Nov 14, 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

4 participants