-
Notifications
You must be signed in to change notification settings - Fork 122
Expand get_items
- deprecate get_all_items
and Catalog.get_item
#1075
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
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1075 +/- ##
=======================================
Coverage 90.25% 90.25%
=======================================
Files 47 47
Lines 6227 6230 +3
Branches 935 934 -1
=======================================
+ Hits 5620 5623 +3
Misses 427 427
Partials 180 180 ☔ View full report in Codecov by Sentry. |
get_items
- deprecate get_all_items
and Catalog.get_item
get_items
- deprecate get_all_items
and Catalog.get_item
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just two minor notebook fixups, otherwise looks good to me. I'm going to ask for another set of eyes on this one, just because it's a non-insignificant API change we're committing too, and I want to make sure it makes sense to someone else.
Oh, needs a CHANGELOG as well.
Co-authored-by: Pete Gadomski <pete.gadomski@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Makes sense to me.
Related Issue(s):
Description:
Instead of using
get_all_items
people should useget_items(recursive=True)
and instead ofCatalog.get_item(id)
people should usenext(Catalog.get_items(id), None)
TO DO:
PR Checklist:
pre-commit
hooks pass locallyscripts/test
)