-
Notifications
You must be signed in to change notification settings - Fork 0
Make "PostBackfillBatchCallback" public #43
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
Coverage Report Results
1 empty file skipped. |
s-cooper18
left a comment
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.
Makes sense
| NoReferencesPrivilege, | ||
| NoReferringTableOwnership, | ||
| NotTableOwner, | ||
| PostBackfillBatchCallback, |
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'm not sure PostBackfillBatchCallback itself needs to be public. I'd assume that type checking would work regardless of this change. What kind of issue has popped up? I wonder if it could be a red herring
One thing I think would be useful to make public is It's already public, I'm a dummy_introspect.BackfillBatch, since that's something users could use when implementing a post backfill batch callback.
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 used PostBackfillBatchCallback here.
Perhaps the justification for it in the description is a bit weak. It's useful if you're declaring your own function that takes an argument with type PostBackfillBatchCallback and passes that through to Psycopack.
One thing I think would be useful to make public is _introspect.BackfillBatch, since that's something users could use when implementing a post backfill batch callback.
It is already :)
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.
Ah got it, you want to use for a type annotation 👍 Makes sense
It is already :)
Yeah I noticed after I wrote it 🤦
This change makes
PostBackfillBatchCallbackpublic, so that whenPsycopackis instantiated with apost_backfill_batch_callbackargument, type checking the argument is possible.