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.

Sign up for GitHub

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

Jump to bottom

DM-39514: Fix misleading doc for Registry.queryDatasetAssociations. #883

Merged
merged 1 commit into from
Aug 18, 2023
Merged

DM-39514: Fix misleading doc for Registry.queryDatasetAssociations. #883

merged 1 commit into from
Aug 18, 2023

Conversation

kfindeisen
Copy link
Member

This PR corrects the docstring for queryDatasetAssociations so that it is consistent with its actual (and desired) behavior. No subclass docstrings needed to be changed.

@kfindeisen
Copy link
Member Author

I can't reproduce the build_and_test failure on rubin-devl:

(Pdb++) print(json_str)
{"definition": "detector", "record": {"instrument": "HSC", "id": 0, "full_name": "1_53", "name_in_raft": "53", "raft": "1", "purpose": "SCIENCE"}}
(Pdb++) type(r).from_json(json_str, registry=butler.registry)
detector.RecordClass(instrument='HSC', id=0, full_name='1_53', name_in_raft='53', raft='1', purpose='SCIENCE')

timj
timj reviewed Aug 17, 2023
View reviewed changes
python/lsst/daf/butler/core/dimensions/_records.py Outdated
@@ -190,7 +190,7 @@ def direct(
# This method requires tuples as values of the mapping, but JSON
# readers will read things in as lists. Be kind and transparently
# transform to tuples
_recItems = {k: v if type(v) != list else tuple(v) for k, v in record.items()} # type: ignore
_recItems = {k: v if not isinstance(v, list) else tuple(v) for k, v in record.items()}
Copy link
Member

Choose a reason for hiding this comment

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

This is being discussed on #882

@timj
Copy link
Member

timj commented Aug 17, 2023

The test failure is from pydantic v2.2 and there is a fix for it on #880 that you can cherry-pick

@kfindeisen
Copy link
Member Author

The test failure is from pydantic v2.2 and there is a fix for it on #880 that you can cherry-pick

Just to confirm, do you mean the commit titled "Move StrictInt ahead of StrictFloat in model"?

@timj
Copy link
Member

timj commented Aug 18, 2023 •
edited
Loading

I'm happy to take over the PR to sort out the checks -- it's a simple doc change on this PR so you shouldn't have to be fixing up code.

@timj
Copy link
Member

timj commented Aug 18, 2023

@kfindeisen I've merged my PR so a rebase should fix all the problems.

@kfindeisen
Fix misleading doc for Registry.queryDatasetAssociations.
b43f4ab 
The doc said that flattenChains is true by default, when it's false
by default.
@kfindeisen
Copy link
Member Author

Yes, I just noticed. Rerunning scons now...

@kfindeisen
Copy link
Member Author

kfindeisen commented Aug 18, 2023 •
edited
Loading

(Incidentally, the "Check commits can be merged" test doesn't seem to be useful, since it only runs on upload but can't account for later commits to main.)

Misremembered what was actually being tested, never mind.

@codecov
Copy link

codecov bot commented Aug 18, 2023 •
edited
Loading

Codecov Report

Patch and project coverage have no change.

Comparison is base (c6c2f70) 87.71% compared to head (b43f4ab) 87.71%.

Additional details and impacted files 
@@           Coverage Diff           @@
##             main     #883   +/-   ##
=======================================
  Coverage   87.71%   87.71%           
=======================================
  Files         274      274           
  Lines       36117    36117           
  Branches     7553     7553           
=======================================
  Hits        31680    31680           
  Misses       3264     3264           
  Partials     1173     1173
Files Changed Coverage Δ
python/lsst/daf/butler/registry/_registry.py 96.55% <ø> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@kfindeisen kfindeisen merged commit 3fdd9d0 into main Aug 18, 2023
16 checks passed
@kfindeisen kfindeisen deleted the tickets/DM-39514 branch August 18, 2023 20:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@TallJimbo TallJimbo TallJimbo approved these changes

@timj timj timj approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@kfindeisen @timj @TallJimbo