Simplifying glyph stream logic in ps backend #24287
Conversation
|
I'll need some help for coming up with the pytest style unit tests as this is my first contribution here |
There was a problem hiding this comment.
Thank you for opening your first PR into Matplotlib!
If you have not heard from us in a while, please feel free to ping @matplotlib/developers or anyone who has commented on the PR. Most of our reviewers are volunteers and sometimes things fall through the cracks.
You can also join us on gitter for real-time discussion.
For details on testing, writing docs, and our review process, please see the developer guide
We strive to be a welcoming and open project. Please follow our Code of Conduct.
|
It seems like the code is well tested as it is, but that it generates an error. You can download the resulting images here: https://github.com/matplotlib/matplotlib/actions/runs/3344012345 and see what happens. (One interpretation is that the proposed code is not doing exactly the same thing as the previous.) |
Right, I've tried fixing a missing append to the stream in the suggested code. |
| @@ -651,8 +651,9 @@ def draw_text(self, gc, x, y, s, prop, angle, ismath=False, mtext=None): | |||
| kern = font.get_kern_dist_from_name(last_name, name) | |||
| last_name = name | |||
| thisx += kern * scale | |||
| xs_names.append((ps_name, thisx, name)) | |||
There was a problem hiding this comment.
I guess(?) that should just have been stream.append((ps_name, thisx, name)) and xs_names is unused and can be removed?
There was a problem hiding this comment.
Yes that's what I wondered and removed ps_name from the xs_names append and appended them into the stream in a fresh line
There was a problem hiding this comment.
I could remove xs_names completely if its not of any significance here
|
Thanks you @Abhijnan-Bajpai and congratulations on your first merged Matplotlib PR 🎉 Hopefully we will hear from you again! |
Thanks for the merge! Felt great contributing to the project I've used for the longest time! I'll definitely be looking out for more opportunities to contribute here. |
PR Summary
Fixes issue #23965 by converting stream logic to a simpler group by form
Suggested by @tacaswell
Originally posted by @anntzer in #23910 (comment)
PR Checklist
Tests and Styling
flake8-docstringsand runflake8 --docstring-convention=all).