-
-
Notifications
You must be signed in to change notification settings - Fork 7.9k
MAINT: small improvements to the pdf backend #14413
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 our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Instead of the `self.nextFoo` counters. This way the various name sequences are defined closer together and it's impossible to forget to increment the counters.
draw_markers has been implemented, draw_line_collection was removed years ago but draw_quad_mesh could be implemented
and delete a useless comment
@@ -482,21 +511,21 @@ def __init__(self, filename, metadata=None): | |||
if v is not None} | |||
|
|||
self.fontNames = {} # maps filenames to internal font names | |||
self.nextFont = 1 # next free internal font name | |||
self.internalFontName = (Name(f'F{i}') for i in itertools.count(1)) |
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 don't really mind the API break (it's clearly intended to be a private API), but please add an API change note and make the new APIs private.
---------- | ||
|
||
id : int | ||
object id of the stream |
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.
object id of the stream | |
Object id of the stream. |
Generally, parameter descriptions should start with a capital letter and finish with a period, even if they are not complete sentences.
(Also in the following docs).
for `'Creator'`, `'Producer'` and `'CreationDate'`. They | ||
can be removed by setting them to `None`. | ||
""" | ||
self.objectId = itertools.count(1) # consumed by reserveObject |
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.
Maybe better name this
self.objectId = itertools.count(1) # consumed by reserveObject | |
self._object_id_generator = itertools.count(1) # consumed by reserveObject |
?
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 gave the new names _seq
suffixes to keep the lines slightly shorter.
and name the new members like _foo_seq to make them obviously private and to conform to PEP8
The pytest version requirement has changed.
Not a huge deal here, but note that we normally wait for approval from two reviewers (other than the PR author) to merge (except for docs-only changes where one reviewer is enough). |
Oops, sorry! I'll be less hasty in the future. -Jouni
|
No worries :) |
PR Summary
Improve some docstrings and comments, use generators instead of keeping
nextFoo
style countersPR Checklist