Shorter property deprecation. by anntzer · Pull Request #18688 · matplotlib/matplotlib

anntzer · 2020-10-07T23:11:09Z

This lets us write deprecated properties with single-line lambdas, which
is quite a bit more compact (as shown by the line diff).

PR Summary

PR Checklist

Has pytest style unit tests (and pytest passes).
Is Flake 8 compliant (run flake8 on changed files to check).
New features are documented, with examples if plot related.
Documentation is sphinx and numpydoc compliant (the docs should build without error).
Conforms to Matplotlib style conventions (install flake8-docstrings and pydocstyle<4 and run flake8 --docstring-convention=all).
New features have an entry in doc/users/next_whats_new/ (follow instructions in README.rst there).
API changes documented in doc/api/next_api_changes/ (follow instructions in README.rst there).

brunobeltran

Is there a reason you didn't make a helper function (just a private guy) that is explicitly for properties? It looks like literally every instance you changed could have been

from .cbook import _deprecate_prop
...
prop = _deprecate_prop("3.X", lambda self: self.whatever())

instead of

from . import cbook
...
prop = cbook.deprecated("3.X", property(lambda: self.whatever()))

lib/matplotlib/backends/backend_gtk3.py

brunobeltran · 2020-10-07T23:44:28Z

lib/mpl_toolkits/mplot3d/axes3d.py

+    w_xaxis = cbook.deprecated("3.1", alternative="xaxis", pending=True)(
+        lambda self: self.xaxis)
+    w_yaxis = cbook.deprecated("3.1", alternative="yaxis", pending=True)(
+        lambda self: self.yaxis)
+    w_zaxis = cbook.deprecated("3.1", alternative="zaxis", pending=True)(
+        lambda self: self.zaxis)


Forgot the property.

QuLogic · 2020-10-08T00:38:29Z

Lambdas are okay on a single line, but I'm not so sure I'm in favour of some of these changes to multi-line things.

anntzer · 2020-10-08T08:54:43Z

I don't think

foo = cbook.deprecated_property("x.y", lambda self: ...)

is really better (or worse) than

foo = cbook.deprecated("x.y", property(lambda self: ...))

I see this a bit like https://docs.python.org/3/library/abc.html#abc.abstractproperty, which was deprecated in favor of stacking abstractmethod on top of property.

I'm fine with reverting some of the multiline cases.

brunobeltran · 2020-10-08T13:41:55Z

lib/matplotlib/backends/backend_cairo.py

+    fontweights = cbook.deprecated("3.3")(property(lambda self: {
+        100:          cairo.FONT_WEIGHT_NORMAL,
+        200:          cairo.FONT_WEIGHT_NORMAL,
+        300:          cairo.FONT_WEIGHT_NORMAL,
+        400:          cairo.FONT_WEIGHT_NORMAL,
+        500:          cairo.FONT_WEIGHT_NORMAL,
+        600:          cairo.FONT_WEIGHT_BOLD,
+        700:          cairo.FONT_WEIGHT_BOLD,
+        800:          cairo.FONT_WEIGHT_BOLD,
+        900:          cairo.FONT_WEIGHT_BOLD,
+        'ultralight': cairo.FONT_WEIGHT_NORMAL,
+        'light':      cairo.FONT_WEIGHT_NORMAL,
+        'normal':     cairo.FONT_WEIGHT_NORMAL,
+        'medium':     cairo.FONT_WEIGHT_NORMAL,
+        'regular':    cairo.FONT_WEIGHT_NORMAL,
+        'semibold':   cairo.FONT_WEIGHT_BOLD,
+        'bold':       cairo.FONT_WEIGHT_BOLD,
+        'heavy':      cairo.FONT_WEIGHT_BOLD,
+        'ultrabold':  cairo.FONT_WEIGHT_BOLD,
+        'black':      cairo.FONT_WEIGHT_BOLD,
+    }))
+    fontangles = cbook.deprecated("3.3")(property(lambda self: {
+        'italic':  cairo.FONT_SLANT_ITALIC,
+        'normal':  cairo.FONT_SLANT_NORMAL,
+        'oblique': cairo.FONT_SLANT_OBLIQUE,
+    }))
+    mathtext_parser = cbook.deprecated("3.4")(
+        property(lambda self: MathTextParser('Cairo')))


This is the only one that doesn't accomplish your goal IMO of making it "easier" to see the deprecated function as a block. If instead you did

Suggested change

fontweights = cbook.deprecated("3.3")(property(lambda self: {

100: cairo.FONT_WEIGHT_NORMAL,

200: cairo.FONT_WEIGHT_NORMAL,

300: cairo.FONT_WEIGHT_NORMAL,

400: cairo.FONT_WEIGHT_NORMAL,

500: cairo.FONT_WEIGHT_NORMAL,

600: cairo.FONT_WEIGHT_BOLD,

700: cairo.FONT_WEIGHT_BOLD,

800: cairo.FONT_WEIGHT_BOLD,

900: cairo.FONT_WEIGHT_BOLD,

'ultralight': cairo.FONT_WEIGHT_NORMAL,

'light': cairo.FONT_WEIGHT_NORMAL,

'normal': cairo.FONT_WEIGHT_NORMAL,

'medium': cairo.FONT_WEIGHT_NORMAL,

'regular': cairo.FONT_WEIGHT_NORMAL,

'semibold': cairo.FONT_WEIGHT_BOLD,

'bold': cairo.FONT_WEIGHT_BOLD,

'heavy': cairo.FONT_WEIGHT_BOLD,

'ultrabold': cairo.FONT_WEIGHT_BOLD,

'black': cairo.FONT_WEIGHT_BOLD,

}))

fontangles = cbook.deprecated("3.3")(property(lambda self: {

'italic': cairo.FONT_SLANT_ITALIC,

'normal': cairo.FONT_SLANT_NORMAL,

'oblique': cairo.FONT_SLANT_OBLIQUE,

}))

mathtext_parser = cbook.deprecated("3.4")(

property(lambda self: MathTextParser('Cairo')))

fontweights = cbook.deprecated("3.3")(property(lambda self: _fontweights))

fontangles = cbook.deprecated("3.3")(property(lambda self: _fontangles))

mathtext_parser = cbook.deprecated("3.4")(

property(lambda self: MathTextParser('Cairo')))

and added right above the class definition something like

# deprecated font property mappings for RendererCairo _fontweights = { 100: cairo.FONT_WEIGHT_NORMAL, 200: cairo.FONT_WEIGHT_NORMAL, 300: cairo.FONT_WEIGHT_NORMAL, 400: cairo.FONT_WEIGHT_NORMAL, 500: cairo.FONT_WEIGHT_NORMAL, 600: cairo.FONT_WEIGHT_BOLD, 700: cairo.FONT_WEIGHT_BOLD, 800: cairo.FONT_WEIGHT_BOLD, 900: cairo.FONT_WEIGHT_BOLD, 'ultralight': cairo.FONT_WEIGHT_NORMAL, 'light': cairo.FONT_WEIGHT_NORMAL, 'normal': cairo.FONT_WEIGHT_NORMAL, 'medium': cairo.FONT_WEIGHT_NORMAL, 'regular': cairo.FONT_WEIGHT_NORMAL, 'semibold': cairo.FONT_WEIGHT_BOLD, 'bold': cairo.FONT_WEIGHT_BOLD, 'heavy': cairo.FONT_WEIGHT_BOLD, 'ultrabold': cairo.FONT_WEIGHT_BOLD, 'black': cairo.FONT_WEIGHT_BOLD, } _fontangles = { 'italic': cairo.FONT_SLANT_ITALIC, 'normal': cairo.FONT_SLANT_NORMAL, 'oblique': cairo.FONT_SLANT_OBLIQUE, }

then I think you would accomplish your goal.

brunobeltran

My "feeling" is that I might have not even complained about the multiline stuff it it had also been moved to the top of the class, but leaving what looks like a class variable hiding between method definitions felt funny to me.

I agree especially that in places like texmanager this greatly reduces visual clutter.

I feel that just stashing all the deprecated properties into a single block with no blank lines in between makes it easier to ignore that block.

I'd approve if

the "block" was moved above __init__, where class variables are typically defined
fix the RendererCairo case to actually make this block easy to "see" as you're scrolling (see the one comment above)

I think in that case you wouldn't really have to revert any of the multi-line ones to make me happy, but in cases where you've just taken one method of a class and instead made it into a multi-line class variable definition, this might have just made this harder to understand quickly with no real benefit in terms of having things in a "block", since there's just one "thing".

anntzer · 2020-10-08T15:36:53Z

OK, I reverted the cases of single deprecations turning into multiline blocks.

brunobeltran

Not going to die on the "these should go above __init__ hill, since I can just open a separate PR to impose my opinions about this, and I will be wanting this lambda syntax for the deprecation-spree I'm about to go on.

So feel free to ignore, but I marked everywhere where they're not currently above __init__. I would say that there is one place left where you didn't put all the deprecations into a single block, which was your original purpose I think (TextBox in widgets.py).

lib/matplotlib/backend_bases.py

lib/matplotlib/backends/backend_gtk3.py

lib/matplotlib/contour.py

lib/matplotlib/widgets.py

lib/matplotlib/backends/backend_pgf.py

lib/matplotlib/backends/backend_ps.py

anntzer · 2020-10-09T18:23:44Z

Moved all of them up, except for the case where I'm just removing the name parameter in widgets.py.

tacaswell · 2020-10-09T19:51:46Z

lib/matplotlib/backends/backend_cairo.py

-            'normal':  cairo.FONT_SLANT_NORMAL,
-            'oblique': cairo.FONT_SLANT_OBLIQUE,
-        }
+    fontweights = cbook.deprecated("3.3")(property(lambda self: _fontweights))


Suggested change

fontweights = cbook.deprecated("3.3")(property(lambda self: _fontweights))

fontweights = cbook.deprecated("3.3")(property(lambda self: dict(_fontweights)))

tacaswell · 2020-10-09T19:52:39Z

lib/matplotlib/backends/backend_cairo.py

-            'oblique': cairo.FONT_SLANT_OBLIQUE,
-        }
+    fontweights = cbook.deprecated("3.3")(property(lambda self: _fontweights))
+    fontangles = cbook.deprecated("3.3")(property(lambda self: _fontangles))


Suggested change

fontangles = cbook.deprecated("3.3")(property(lambda self: _fontangles))

fontangles = cbook.deprecated("3.3")(property(lambda self: dict(_fontangles)))

This and the line above used to return a fresh dict that the user could modify, this makes sure we do not start leaking mutable global state out.

This lets us write deprecated properties with single-line lambdas, which is quite a bit more compact (as shown by the line diff).

QuLogic · 2020-10-16T07:04:48Z

lib/matplotlib/backends/backend_ps.py

-    @cbook.deprecated("3.3")
-    @property
-    def mathtext_parser(self):
-        return MathTextParser("PS")


This used to be the wrong 'since' version, I guess?

yes (#18002)

brunobeltran reviewed Oct 7, 2020

View reviewed changes

anntzer force-pushed the depprop branch from aa964d8 to 3c0ad09 Compare October 8, 2020 08:51

brunobeltran reviewed Oct 8, 2020

View reviewed changes

anntzer force-pushed the depprop branch from 3c0ad09 to 2958fe0 Compare October 8, 2020 15:36

brunobeltran approved these changes Oct 9, 2020

View reviewed changes

anntzer force-pushed the depprop branch from 2958fe0 to 69607f9 Compare October 9, 2020 18:23

tacaswell reviewed Oct 9, 2020

View reviewed changes

Shorter property deprecation.

da7c0dd

This lets us write deprecated properties with single-line lambdas, which is quite a bit more compact (as shown by the line diff).

anntzer force-pushed the depprop branch from 69607f9 to da7c0dd Compare October 10, 2020 10:17

QuLogic approved these changes Oct 16, 2020

View reviewed changes

QuLogic merged commit cf83ec4 into matplotlib:master Oct 17, 2020

QuLogic added this to the v3.4.0 milestone Oct 17, 2020

QuLogic added the Maintenance label Oct 17, 2020

anntzer deleted the depprop branch October 17, 2020 08:44

	fontweights = cbook.deprecated("3.3")(property(lambda self: _fontweights))
	fontweights = cbook.deprecated("3.3")(property(lambda self: dict(_fontweights)))

	fontangles = cbook.deprecated("3.3")(property(lambda self: _fontangles))
	fontangles = cbook.deprecated("3.3")(property(lambda self: dict(_fontangles)))

Uh oh!

Conversation

anntzer commented Oct 7, 2020

PR Summary

PR Checklist

Uh oh!

brunobeltran left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

QuLogic commented Oct 8, 2020

Uh oh!

anntzer commented Oct 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brunobeltran Oct 8, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

brunobeltran left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

anntzer commented Oct 8, 2020

Uh oh!

brunobeltran left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

anntzer commented Oct 9, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

anntzer commented Oct 8, 2020 •

edited

Loading

brunobeltran Oct 8, 2020 •

edited

Loading

brunobeltran left a comment •

edited

Loading