X Tutup
Skip to content

Missing return in _num_to_string()#8594

Merged
tacaswell merged 1 commit intomatplotlib:masterfrom
FedeMiorelli:patch-1
May 9, 2017
Merged

Missing return in _num_to_string()#8594
tacaswell merged 1 commit intomatplotlib:masterfrom
FedeMiorelli:patch-1

Conversation

@FedeMiorelli
Copy link
Copy Markdown
Contributor

Added a missing return statement that caused "None" strings to be displayed with LogFormatter.

PR Summary

PR Checklist

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

Added a missing return statement that caused "None" strings to be displayed with LogFormatter.
@WeatherGod
Copy link
Copy Markdown
Member

I am a bit concerned that nobody has noticed this before? This method appears to be used in the LogFormatter's __call__ method!

@story645
Copy link
Copy Markdown
Member

story645 commented May 8, 2017

I've noticed it before with other formatters but thought it was expected behavior. Wonder if that's what's going on?

@FedeMiorelli can you add a test that this now yields the expected labeling? You can add it to the LogFormatter tests.

@tacaswell tacaswell added this to the 2.0.2 (critical bug fixes from 2.0.1) milestone May 9, 2017
@tacaswell
Copy link
Copy Markdown
Member

That does appear to be very broken...

Looks like it was broken in 8bdabcd

The tests we have of this either a) test fmt.pprint_val directly or b) only check that tick labels are non-empty strings or not.

Going to merge, backport this, and create a new issue for adding the tests.

@tacaswell tacaswell merged commit 74fa27b into matplotlib:master May 9, 2017
tacaswell added a commit that referenced this pull request May 9, 2017
FIX: Missing return in LogFormatter._num_to_string()
@tacaswell
Copy link
Copy Markdown
Member

backported to v2.0.x as e273f7e

@efiring
Copy link
Copy Markdown
Member

efiring commented May 9, 2017

Wow! I really blew that one. Thanks for finding and fixing it.

@FedeMiorelli FedeMiorelli deleted the patch-1 branch May 9, 2017 15:27
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.

5 participants

X Tutup