Skip to content

MarkerStyle is considered immutable#19341

Merged
jklymak merged 1 commit intomatplotlib:masterfrom
timhoffm:immutable-marker
Jan 28, 2021
Merged

MarkerStyle is considered immutable#19341
jklymak merged 1 commit intomatplotlib:masterfrom
timhoffm:immutable-marker

Conversation

@timhoffm
Copy link
Copy Markdown
Member

@timhoffm timhoffm commented Jan 22, 2021 •
edited
Loading

PR Summary

as discussed in yesterdays dev call.

See also #17850 (comment).

@timhoffm timhoffm added this to the v3.4.0 milestone Jan 22, 2021
@timhoffm timhoffm force-pushed the immutable-marker branch 7 times, most recently from 475462a to d27ab97 Compare January 22, 2021 23:13
anntzer
anntzer reviewed Jan 24, 2021
View reviewed changes
Comment thread lib/matplotlib/markers.py Outdated
anntzer
anntzer reviewed Jan 24, 2021
View reviewed changes
Comment thread lib/matplotlib/tests/test_marker.py Outdated
jklymak
jklymak reviewed Jan 25, 2021
View reviewed changes
Copy link
Copy Markdown
Member

@jklymak jklymak left a comment

Choose a reason for hiding this comment

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

Overall makes sense to me, but it seems lines.set_fillstyle is not tested? Or did you want to deprecate that as well?

Comment thread lib/matplotlib/lines.py
For examples see :ref:`marker_fill_styles`.
"""
self._marker.set_fillstyle(fs)
self.set_marker(MarkerStyle(self._marker.get_marker(), fs))
Copy link
Copy Markdown
Member

@jklymak jklymak Jan 25, 2021

Choose a reason for hiding this comment

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

I guess we never test set_fillstyle? Can we use this as the opportunity to do so?

Copy link
Copy Markdown
Member Author

@timhoffm timhoffm Jan 25, 2021

Choose a reason for hiding this comment

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

I'd like to have this deprecation be part of 3.4 and I don't know if I have the bandwidth to add tests soon. Therefore, I'd rather do this in a later PR.

@timhoffm
Copy link
Copy Markdown
Member Author

timhoffm commented Jan 25, 2021 •
edited
Loading

As opposed to what I said in the dev call, we cannot easily deprecate Lines.set/get_fillstyle. I thought get_marker() would return the MarkerStyle so that we could do line.get_marker().get_fillstyle(). However, the used MarkerStyle object is not public. get_marker() does not return it but only the marker symbol, e.g. 'o'.

Therefore, I'll leave marker and fillstyle as "simple" Matplotlib properties of Line2D for now.

@jklymak jklymak merged commit 476b7e4 into matplotlib:master Jan 28, 2021
@timhoffm timhoffm deleted the immutable-marker branch January 28, 2021 21:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@anntzer anntzer anntzer approved these changes

@jklymak jklymak jklymak approved these changes

Assignees

No one assigned

Labels

API: changes topic: collections and mappables

Projects

None yet

Milestone

v3.4.0

Development

Successfully merging this pull request may close these issues.

4 participants

@timhoffm @anntzer @jklymak @QuLogic