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-33892: Add TractTableValueMeasurementTask #134

Merged
merged 2 commits into from May 23, 2022
Merged

DM-33892: Add TractTableValueMeasurementTask #134

merged 2 commits into from May 23, 2022

Conversation

taranu
Copy link
Contributor

@taranu taranu commented Apr 26, 2022

No description provided.

@taranu taranu force-pushed the tickets/DM-33892 branch 2 times, most recently from 7053225 to c8ae0d0 Compare April 29, 2022 17:27
for column in columns_base)

# TODO: does it make sense to validate this now or just leave it to the run call?
# assert all(column in inputs['columns'] for column in columns_in)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps this check is more appropriate here? It seems more like a validation of the formatting of the inputs to the run method rather than something fundamental to the calculation itself.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah sorry, I should clarify - if any of columns_in are not in the table, then the call to inputs['table'].get(parameters={'columns': columns_in}) three lines later will fail. So I figured there's not really any point in duplicating a check that the get method does.

Copy link
Collaborator

@bechtol bechtol left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. I added some general comments on the ticket. No requested changes changes at this time.

@taranu taranu force-pushed the tickets/DM-33892 branch 2 times, most recently from d2c17dd to a62c4eb Compare May 19, 2022 02:55
@taranu taranu merged commit 848d8ff into main May 23, 2022
@taranu taranu deleted the tickets/DM-33892 branch May 23, 2022 14:02
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.

None yet

2 participants