-
Notifications
You must be signed in to change notification settings - Fork 12
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
DM-29761: start saving per-quantum provenance and propagating nothing-to-do cases #183
base: main
Are you sure you want to change the base?
Conversation
Everything else in a PipelineTask implementation already worked with connection names, not dataset type names. WIP: remove the optional return stuff from this and put it on a separate commit.
This will allow code in ctrl_mpexec to delegate to adjustQuantum before calling runQuantum, to test whether running is necessary in the exact same way it is tested at QG generation time.
97996dc
to
af16b87
Compare
@@ -547,6 +547,24 @@ def translateAdjustQuantumInputs( | |||
) | |||
return results | |||
|
|||
def hasPostWriteLogic(self): |
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.
Why is this a method?
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 just never know what to do with boolean flag getters; I feel like "starts with verb" implies "it's a method", but there's pretty much no other way to name boolean properties. I'm not even consistent about that within this commit.
Everybody else in the world, please just agree on what we should do in this case and I'll happily follow along.
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.
Is the implied rest of the question "and not a property"?
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.
Or just an attribute, That's the great thing about properties is that they can be migrated or substituted in place of an attribute if needed
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 one shouldn't be an attribute, because I think the right specialization pattern is "override method [or property]".
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.
Why is that better than setting a class level attribute? I really don't understand why this is such an uncommon thing. Are you trying to protect it from changing, and if so other code would be needed here.
No description provided.