Repository navigation
Store dash_pattern as single attribute, not two. - #21050
timhoffm merged 1 commit into
Conversation
Sorry, something went wrong.
| # this is needed scaling the dash pattern by linewidth | ||
| self._us_dashSeq = None | ||
| self._us_dashOffset = 0 | ||
| self._us_dash_pattern = (0, None) # offset, dash (unscaled) |
There was a problem hiding this comment.
I‘m inclined to use a namedtuple instead of the comment for semantic clarity.
Sorry, something went wrong.
There was a problem hiding this comment.
At that point, should it be a dataclass so that it can eventually carry documentation/the linestyle docs can be pushed there?
Sorry, something went wrong.
There was a problem hiding this comment.
You could write self._dash_pattern = DashPattern(0, None) but I would argue that this is actually worse and still needs a comment (or DashPattern(offset=0, dashes=None) to not need a comment), as it is less clear that we're just taking two values and not doing anything fancy with them (whereas a constructor could do arbitrary things with its arguments).
Additionally, we'd never actually use the .offset and .dashes accessors, which removes the main benefit of having named fields (well, _scale_dashes could take a packed dashpattern rather than an unpacked dashpattern as parameter, but that doesn't seem like a big deal).
Using a dataclass would also makes things harder on the C (agg) side for parsing (well, it's always doable, but you get a bunch of PyObject_GetAttrString and PyFloat_AsDouble and error and refcount management and whatnot, versus PyArg_ParseTuple which does many things for you).
Sorry, something went wrong.
| @@ -603,8 +598,9 @@ def draw(self, renderer): | |||
| if not self.get_visible(): | |||
| return | |||
| # Patch has traditionally ignored the dashoffset. | |||
There was a problem hiding this comment.
Why?
Sorry, something went wrong.
There was a problem hiding this comment.
"backcompat".
Sorry, something went wrong.
|
I'm not really sure this is better than just having two attributes? *-Expanding the tuple all the time doesn't improve readability. |
Sorry, something went wrong.
|
I think(?) storing as a single tuple makes it clearer that the two things are closely related and should basically always be considered together. |
Sorry, something went wrong.
|
This PR is affected by a re-writing of our history to remove a large number of accidentally committed files see discourse for details. To recover this PR it will need be rebased onto the new default branch (main). There are several ways to accomplish this, but we recommend (assuming that you call the matplotlib/matplotlib remote "upstream" git remote update
git checkout main
git merge --ff-only upstream/main
git checkout YOUR_BRANCH
git rebase --onto=main upstream/old_master
# git rebase -i main # if you prefer
git push --force-with-lease # assuming you are tracking your branchIf you do not feel comfortable doing this or need any help please reach out to any of the Matplotlib developers. We can either help you with the process or do it for you. Thank you for your contributions to Matplotlib and sorry for the inconvenience. |
Sorry, something went wrong.
timhoffm
left a comment
There was a problem hiding this comment.
I'm still a bit uneasy about using a simple tuple because that is not self-explanatory; in contrast to two variables or a namedtuple. But maybe ok because the only usages are definitions (with comment) and unpacking into functions.
Sorry, something went wrong.
Dash offset and pattern are always used together, so just store them as single attributes on lines and patches, instead of separate ones.
Dash offset and pattern are always used together, so just store them as
single attributes on lines and patches, instead of separate ones.
PR Summary
PR Checklist