Inline or simplify FooFormatter.pprint_val. #12950
Conversation
This avoids confusion between the three implementations (OldScalarFormatter, ScalarFormatter, and LogFormatter). OldScalarFormatter._pprint_val and ScalarFormatter._pprint_val are not used elsewhere and can be inlined. LogFormatter._pprint_val is also used by LogFormatterExponent and must be kept (even though it looks likely that LogFormatterExponent can also be simplified , e.g. the test `abs(math.log(x) / math.log(self._base)) > 10000` is basically never True for any realistic case -- but that'll be for another time).
|
-0.5 on inlineing. Pretty-printing takes a number of lines of code and is a semantic operation of it's own. Therefore it deserves its own function, even if it's just used once. I would be positive on unifying the API, or maybe moving it to a cbook function. |
|
The problem is that different formatters can't even agree on what pretty-printing means, even though it's pretty much orthogonal to their functionality: Why does ScalarFormatter.pprint_val obey useLocale but not OldScalarFormatter.pprint_val or LogFormatter.pprint_val? Why does OldScalarFormatter.pprint_val strip the + sign from the exponent but not LogFormatter? |
|
I agree with your assessment of the current state. However, my recommendation to changing a bad code structure is to make a better structure, not to throw away all structure. |
|
Well, the good thing is that at least now the structure is private, so we can play with it in whatever way we want... |
|
Lets merge and if we want to re-factor privately we can... |
…950-on-v3.1.x Backport PR #12950 on branch v3.1.x (Inline or simplify FooFormatter.pprint_val.)
This avoids confusion between the three implementations
(OldScalarFormatter, ScalarFormatter, and LogFormatter).
OldScalarFormatter._pprint_val and ScalarFormatter._pprint_val are not
used elsewhere and can be inlined.
LogFormatter._pprint_val is also used by LogFormatterExponent and must
be kept (even though it looks likely that LogFormatterExponent can also
be simplified , e.g. the test
abs(math.log(x) / math.log(self._base)) > 10000is basically never True for any realistic case -- but that'll be for
another time).
Followup to #12924.
PR Summary
PR Checklist