Skip to content
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

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

TallJimbo
Copy link
Member

No description provided.

TallJimbo added 5 commits May 19, 2021 13:30
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.
@@ -547,6 +547,24 @@ def translateAdjustQuantumInputs(
)
return results

def hasPostWriteLogic(self):
Copy link
Contributor

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?

Copy link
Member Author

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.

Copy link
Member

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"?

Copy link
Contributor

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

Copy link
Member Author

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]".

Copy link
Contributor

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants