FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Simplifying glyph stream logic in ps backend by 404-P4rziv4L · Pull Request #24287 · matplotlib/matplotlib · GitHub

  1. matplotlib
  2. matplotlib

Repository navigation

Simplifying glyph stream logic in ps backend - #24287

Merged
tacaswell merged 4 commits into
matplotlib:mainfrom
404-P4rziv4L:main
Oct 28, 2022
ConversationCommits (4)ChecksFiles changed
Merged

Simplifying glyph stream logic in ps backend#24287
tacaswell merged 4 commits into
matplotlib:mainfrom
404-P4rziv4L:main

Conversation

404-P4rziv4L commented Oct 28, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor

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

  • Is Flake 8 compliant (install flake8-docstrings and run flake8 --docstring-convention=all).

404-P4rziv4L commented Oct 28, 2022

Copy link
Copy Markdown
Contributor Author

I'll need some help for coming up with the pytest style unit tests as this is my first contribution here

github-actions Bot reviewed Oct 28, 2022
View reviewed changes

github-actions Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

oscargus commented Oct 28, 2022

Copy link
Copy Markdown
Member

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.)

404-P4rziv4L commented Oct 28, 2022

Copy link
Copy Markdown
Contributor Author

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.

anntzer reviewed Oct 28, 2022
View reviewed changes
Comment thread lib/matplotlib/backends/backend_ps.py Outdated
kern = font.get_kern_dist_from_name(last_name, name)
last_name = name
thisx += kern * scale
xs_names.append((ps_name, thisx, name))

anntzer Oct 28, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I guess(?) that should just have been stream.append((ps_name, thisx, name)) and xs_names is unused and can be removed?

404-P4rziv4L Oct 28, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

404-P4rziv4L Oct 28, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I could remove xs_names completely if its not of any significance here

oscargus approved these changes Oct 28, 2022
View reviewed changes

oscargus left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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.

oscargus added this to the v3.7.0 milestone Oct 28, 2022
tacaswell merged commit ee6fc0f into matplotlib:main Oct 28, 2022

tacaswell commented Oct 28, 2022

Copy link
Copy Markdown
Member

Thanks you @Abhijnan-Bajpai and congratulations on your first merged Matplotlib PR 🎉 Hopefully we will hear from you again!

404-P4rziv4L commented Oct 28, 2022

Copy link
Copy Markdown
Contributor Author

Thanks you @Abhijnan-Bajpai and congratulations on your first merged Matplotlib PR tada 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.

oscargus mentioned this pull request Nov 3, 2022
Closed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

anntzer anntzer left review comments
github-actions[bot] github-actions[bot] left review comments
tacaswell tacaswell approved these changes
oscargus oscargus approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

v3.7.0

Development

Successfully merging this pull request may close these issues.

4 participants

Back | FazBrowse Home | New Git URL