Skip to content

Make FontManager.score_weight less lenient. - #12565

Merged
jklymak merged 1 commit into
matplotlib:masterfrom
anntzer:score_weight
Nov 11, 2018
Merged

Make FontManager.score_weight less lenient.#12565
jklymak merged 1 commit into
matplotlib:masterfrom
anntzer:score_weight

Conversation

@anntzer

@anntzer anntzer commented Oct 19, 2018

Copy link
Copy Markdown
Contributor

PR Summary

PR Checklist

  • Has Pytest style unit tests
  • Code is Flake 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@anntzer anntzer added this to the v3.1 milestone Oct 19, 2018
@dstansby

dstansby commented Oct 22, 2018

Copy link
Copy Markdown
Member

What's the reason/rationale for this change? And does it affect user functions such as ax.text()?

@anntzer

anntzer commented Oct 22, 2018

Copy link
Copy Markdown
Contributor Author

Silently replacing invalid values by 500 just seems like a good way to shoot onself in the foot.
While score_weight is technically public API, it probably only ever gets called by the fontconfig-pattern-matcher, and the fontconfig-pattern-parser already checks that the pattern is correct, so the PR is unlikely to affect real use cases.

jklymak
jklymak reviewed Nov 11, 2018
View reviewed changes
Comment thread lib/matplotlib/font_manager.py
weightval2 = weight_dict.get(weight2, 500)
return 0.95*(abs(weightval1 - weightval2) / 1000.0) + 0.05
w1 = weight1 if isinstance(weight1, Number) else weight_dict[weight1]
w2 = weight2 if isinstance(weight2, Number) else weight_dict[weight2]

@jklymak jklymak Nov 11, 2018

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Agree w the change. A little concerned that the error message will be mysterious? What is the actual error message here?

@anntzer anntzer Nov 11, 2018

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

They'd get a KeyError on the failing dict lookup, which may not be the nicest but should not be too confusing either. Frankly I'd rather wait for some version of #12232 (up to API discussion) to go in rather than add more custom error messages to this kind of things...

@jklymak
jklymak merged commit 9b01a03 into matplotlib:master Nov 11, 2018
@anntzer
anntzer deleted the score_weight branch November 11, 2018 17:58
@anntzer anntzer mentioned this pull request Dec 14, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@jklymak jklymak jklymak approved these changes
@timhoffm timhoffm timhoffm approved these changes

Assignees

No one assigned

Labels

topic: text

Projects

None yet

Milestone

v3.1.0

Development

Successfully merging this pull request may close these issues.

4 participants

@anntzer @dstansby @jklymak @timhoffm