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-13944: add id to VisitInfo #382

Merged
merged 2 commits into from Jun 18, 2021
Merged

DM-13944: add id to VisitInfo #382

merged 2 commits into from Jun 18, 2021

Conversation

parejkoj
Copy link
Collaborator

No description provided.

Copy link
Member

@timj timj left a comment

Choose a reason for hiding this comment

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

Looks okay. My main worry is now the legacy interface is different to the new one.

@@ -78,7 +78,7 @@ def __init__(self, log=None, doStripHeader=False):
self.log = log
self.doStripHeader = doStripHeader

def __call__(self, md, exposureId):
def __call__(self, md, exposureId=0, id=0):
Copy link
Member

Choose a reason for hiding this comment

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

I think the only code that calls this now is in obs_test (in addition to a test of it in obs_base) so we may want to consider deleting it completely now. The issue is that it's adding a new optional parameter id that makes it incompatible with the ObservationInfo implementation (which already ignores the parameter). If you retain it I think you are going to have to add id to the other one as well (and maybe issue a warning if you get given it). Alternatively, don't add this parameter here at all and always set it to 0 inside because there isn't a situation where people are going to be passing it in.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is a good point: I've reverted this commit. DM-13943 will deprecate exposureId, and we can make it an optional kwarg then.

@parejkoj parejkoj merged commit 03d0202 into master Jun 18, 2021
@parejkoj parejkoj deleted the tickets/DM-13944 branch June 18, 2021 18:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants