-
Notifications
You must be signed in to change notification settings - Fork 1
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
Allow manual updates #65
Conversation
for more information, see https://pre-commit.ci
…m_publisher into manual_updates
for more information, see https://pre-commit.ci
Since this is the first PR after initial release, should probably include a CHANGELOG entry |
for more information, see https://pre-commit.ci
@@ -31,7 +31,7 @@ def on_progress_update(progress_update: dict): | |||
""" | |||
self.announce(message=dict(**progress_update, **additional_metadata)) | |||
|
|||
return TQDMProgressSubscriber(iterable=iterable, on_progress_update=on_progress_update, **tqdm_kwargs) | |||
return TQDMProgressSubscriber(*tqdm_args, on_progress_update=on_progress_update, **tqdm_kwargs) |
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.
return TQDMProgressSubscriber(*tqdm_args, on_progress_update=on_progress_update, **tqdm_kwargs) | |
return TQDMProgressSubscriber(*tqdm_args, *, on_progress_update=on_progress_update, **tqdm_kwargs) |
Actually, to match the statement in the CHANGELOG (required), need this star 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.
This does not seem acceptable when instantiating a class. Is this where you meant to put this?
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.
No, meant in the init
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.
This is already handled by the *tqdm_args
argument, right?
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.
No, *
says 'everything to the right of this position is required to be a keyword argument only'
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.
*
by itself that is, *args
just unpacks a list of ordered unnamed positional values to the signature
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.
Actually I guess you're right, if it sees *args
it imposes keyword only to the right of that
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #65 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 5 5
Lines 73 72 -1
=========================================
- Hits 73 72 -1
Flags with carried forward coverage won't be shown. Click here to find out more.
|
This PR makes the
TQDMProgressSubscriber
class compatible with manual updates to thetqdm
class, as used in hdmf-dev/hdmf#1110.