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-33204: Add deprecation warning for ButlerURI #629

Merged
merged 3 commits into from Jan 13, 2022
Merged

DM-33204: Add deprecation warning for ButlerURI #629

merged 3 commits into from Jan 13, 2022

Conversation

timj
Copy link
Member

@timj timj commented Jan 10, 2022 •
edited

The code is a bit hairy...

@TallJimbo let me know if you have any better ideas, especially regarding the subclasses trying to instantiate something.

Checklist

  • ran Jenkins
  • added a release note for user-visible changes to doc/changes

@timj timj requested a review from TallJimbo January 10, 2022 23:04
@codecov
Copy link

codecov bot commented Jan 10, 2022 •
edited

Codecov Report

Merging #629 (96c6f0e) into main (313a0bf) will increase coverage by 0.00%.
The diff coverage is 96.15%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #629   +/-   ##
=======================================
  Coverage   84.11%   84.12%           
=======================================
  Files         237      237           
  Lines       30050    30075   +25     
  Branches     4990     4996    +6     
=======================================
+ Hits        25277    25301   +24     
  Misses       3635     3635           
- Partials     1138     1139    +1
Impacted Files Coverage Δ
python/lsst/daf/butler/core/_butlerUri.py 96.00% <96.00%> (-4.00%) ⬇️
tests/test_uri.py 99.01% <100.00%> (+<0.01%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 313a0bf...96c6f0e. Read the comment docs.

TallJimbo
TallJimbo approved these changes Jan 11, 2022
View reviewed changes
Copy link
Member

@TallJimbo TallJimbo left a comment

Choose a reason for hiding this comment

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

I don't really have an opinion about whether this is better or worse than no deprecation warning at all, but I don't have any better ideas. I spent a few minutes researching import hooks in the hopes of finding a way to emit a warning when the symbol is imported or accessed as an attribute of a module, but came up empty-handed.

@timj
Copy link
Member Author

timj commented Jan 11, 2022

There can't possibly be many ButlerURI users out in the wild so this should be fine even if it's a bit convoluted.

@timj
Create new ButlerURI class with deprecation message
ac16e0e 
This implementation does not currently support people
using `isinstance(uri, ButlerURI)` since it always
returns a ResourcePath.
@timj
Fix ButlerURI inheritance
e3a2967 
This change inserts ButlerURI into the ResourcePath class
inheritance by repointing the base class.
@timj
Deprecated package is now a dependency
96c6f0e
@timj
Copy link
Member Author

timj commented Jan 11, 2022

Looks like I can't merge this until pipe_base loses ButlerURI -- mostly because one of the tests in pipelines_check is getting confused by the warnings come out of pipe_base (although I'll take a quick look at the test and see if it's a real problem).

@TallJimbo
Copy link
Member

pipe_base changes are pretty thoroughly backed up behind other changes now. If you want to move faster feel free to convert it (by cherry-picking b014171b73c1271bfc3db8df568fcff7fa6d8931 or otherwise) and I'll rebase.

@timj
Copy link
Member Author

timj commented Jan 11, 2022

I'll take a quick look at that.

@timj timj merged commit 294c620 into main Jan 13, 2022
@timj timj deleted the tickets/DM-33204 branch January 13, 2022 04:30
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

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

Successfully merging this pull request may close these issues.

None yet
2 participants
@timj @TallJimbo