XMLWordPrintable

#### Details

• Type: Story
• Status: Done
• Resolution: Done
• Fix Version/s: None
• Component/s:
• Labels:
None
• Story Points:
1
• Sprint:
DB_S17_6
• Team:
Data Access and Database

#### Description

The queryMetadata methods in both Mapper and Butler are very badly documented. Their docs both claim to accept a "key" parameter, but the methods don't take "key" (and Butler's version attempts to use one if "format" is not specified), while the "format" parameter (which appears non-optional from the code itself) doesn't have any docstring in Mapper. Looks like these docs rotted badly.

#### Activity

Hide
Colin Slater added a comment -

Bumping this. I run into this frequently and every time I have to go read the code to figure out how to use this function.

Show
Colin Slater added a comment - Bumping this. I run into this frequently and every time I have to go read the code to figure out how to use this function.
Hide
Tim Jenness added a comment -

I've marked DM-9255 as a duplicate of this and removed the default assignee of Kian-Tat Lim.

Show
Tim Jenness added a comment - I've marked DM-9255 as a duplicate of this and removed the default assignee of Kian-Tat Lim .
Hide
John Parejko added a comment -

Pinging this one again: we've started updating obs packages to the tests in obs_base, and I was reminded again of queryMetadata's brokenness.

Show
John Parejko added a comment - Pinging this one again: we've started updating obs packages to the tests in obs_base, and I was reminded again of queryMetadata 's brokenness.
Hide
Nate Pease [X] (Inactive) added a comment - - edited

Kian-Tat Lim I need some guidance here:

In march 2016 I removed ‘key’ from the queryMetadata arguments, because ultimately it got passed to Mapper.queryMetadata but that function did not use it (and CameraMapper does not override queryMetadata, where it could have used it).

The docstring needs to be updated, this is trivial.

But, inside Butler.queryMetadata, if the input argument ‘format’ is none, there is code that tries to assign key to format (format = (key,)),

this should have been removed at the same time as key was removed. I’d like to do that now, but I’d like to verify what should be done with format. I think the best thing is to remove the default value of format (None), and make it a required argument. I think this makes sense; if user code is not crashing because key is undefined then all users are passing format to queryMetadata anyway. If that is not the right way to go, I need to know what should be done.

Show
Nate Pease [X] (Inactive) added a comment - - edited Kian-Tat Lim I need some guidance here: In march 2016 I removed ‘key’ from the queryMetadata arguments, because ultimately it got passed to Mapper.queryMetadata but that function did not use it (and CameraMapper does not override queryMetadata, where it could have used it). The docstring needs to be updated, this is trivial. But, inside Butler.queryMetadata, if the input argument ‘format’ is none, there is code that tries to assign key to format (format = (key,)), this should have been removed at the same time as key was removed. I’d like to do that now, but I’d like to verify what should be done with format. I think the best thing is to remove the default value of format (None), and make it a required argument. I think this makes sense; if user code is not crashing because key is undefined then all users are passing format to queryMetadata anyway. If that is not the right way to go, I need to know what should be done.
Hide
Kian-Tat Lim added a comment -

That sounds right. key initially was supposed to be like level in the mapper policy, but in practice there is no reason to have it because the level of granularity is implicitly defined by the format. format is then required and not optional.

Show
Kian-Tat Lim added a comment - That sounds right. key initially was supposed to be like level in the mapper policy, but in practice there is no reason to have it because the level of granularity is implicitly defined by the format . format is then required and not optional.
Hide
Nate Pease [X] (Inactive) added a comment -

K-T, since you're the most familiar with query metadata can you please review this? There are only a few lines of changes, so I hope it won't be much work for you.

Show
Nate Pease [X] (Inactive) added a comment - K-T, since you're the most familiar with query metadata can you please review this? There are only a few lines of changes, so I hope it won't be much work for you.
Hide
Kian-Tat Lim added a comment -

LGTM.

Show
Kian-Tat Lim added a comment - LGTM.

#### People

Assignee:
Nate Pease [X] (Inactive)
Reporter:
John Parejko
Reviewers:
Kian-Tat Lim
Watchers:
Colin Slater, Hsin-Fang Chiang, John Parejko, John Swinbank, Kian-Tat Lim, Nate Pease [X] (Inactive), Paul Price, Russell Owen, Simon Krughoff, Tim Jenness