Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork7.9k
Simplifying glyph stream logic in ps backend#24287
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 uson gitter for real-time discussion.
For details on testing, writing docs, and our review process, please seethe developer guide
We strive to be a welcoming and open project. Please follow ourCode 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Yes that's what I wondered and removedps_name from thexs_names append and appended them into the stream in a fresh line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I could remove xs_names completely if its not of any significance here
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Not sure if anyone can figure out a test case that will stress the code beyond the current ones, but as this is a refactoring and the existing tests passes (with full coverage on the changed lines), I think it should be OK.
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. |
Uh oh!
There was an error while loading.Please reload this page.
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).