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

Implement new ItemSearch result consumers #284

Merged

Conversation

TomAugspurger
Copy link
Collaborator

@TomAugspurger TomAugspurger commented Jul 28, 2022

Closes #285


(original description)

Followup to #277 and
closes #279

the cli.search and get_all_items() / item_collection() were using
this deprecated method. Users using non-deprecated APIs shouldn't be
getting deprecation warnings.

Clearly it's useful, so let's either un-deprecate it or move the
implementaiton to a private method and call that directly. I've chosen
to un-deprecate it.

I've also configured pytest to fail on warnings, to catch this type of
issue in CI.

Followup to stac-utils#277 and
closes stac-utils#279

the `cli.search` and `get_all_items()` / `item_collection()` were using
this deprecated method. Users using non-deprecated APIs shouldn't be
getting deprecation warnings.

Clearly it's useful, so let's either un-deprecate it or move the
implementaiton to a private method and call that directly. I've chosen
to un-deprecate it.

I've also configured pytest to fail on warnings, to catch this type of
issue in CI.
@TomAugspurger
Copy link
Collaborator Author

I've repurposed this PR to implement #285, since it would have created a merge conflict anyway.

I've slipped in a couple additional changes which I'll point out inline.

@TomAugspurger TomAugspurger changed the title Un-deprecate get_all_items_as_dict Implement new ItemSearch result consumers Jul 29, 2022
@codecov-commenter
Copy link

codecov-commenter commented Jul 29, 2022

Codecov Report

Merging #284 (91ddbb0) into main (004428f) will increase coverage by 0.53%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #284      +/-   ##
==========================================
+ Coverage   84.36%   84.90%   +0.53%     
==========================================
  Files          11       11              
  Lines         774      775       +1     
==========================================
+ Hits          653      658       +5     
+ Misses        121      117       -4     
Impacted Files Coverage Δ
pystac_client/cli.py 60.31% <100.00%> (ø)
pystac_client/item_search.py 92.83% <100.00%> (+1.21%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

Tom Augspurger added 2 commits August 1, 2022 07:32
Copy link
Member

@gadomski gadomski left a comment

Choose a reason for hiding this comment

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

A couple of inline comments, and two top-level

  • I really like the way things chain down to pages_as_dict. This feels very natural and will allow good control in the future over fetching strategies, etc.
  • Should we commit to a removal schedule for the deprecated methods (e.g. v0.6.0)?

I'm going to request @matthewhanson's review as well since this is a relatively significant architectural shift.

@gadomski gadomski requested a review from matthewhanson August 1, 2022 21:51
Tom Augspurger added 2 commits August 2, 2022 09:02
@gadomski gadomski self-requested a review August 2, 2022 15:30
@gadomski gadomski added this to the 0.4.1 milestone Aug 2, 2022
@matthewhanson
Copy link
Member

I like these changes, the new function names are intuitive and descriptive.

@TomAugspurger
Copy link
Collaborator Author

TomAugspurger commented Aug 5, 2022

Should we commit to a removal schedule for the deprecated methods (e.g. v0.6.0)?

Sure. This came up before, but I'd like at least one release where the warnings are elevated to FutureWarning since the warnings aren't visible to users working interactively at the moment:

In [1]: import pystac_client

In [2]: catalog = pystac_client.Client.open("https://planetarycomputer.microsoft.com/api/stac/v1")

In [3]: search = catalog.search(collections=["sentinel-2-l2a"], datetime="2022-01-01")

In [4]: search.get_item_collections()
Out[4]: <generator object ItemSearch.pages at 0x7f72b3542580>

@gadomski gadomski self-requested a review August 8, 2022 15:13
@gadomski gadomski modified the milestones: 0.4.1, 0.5.0 Aug 8, 2022
Copy link
Member

@gadomski gadomski left a comment

Choose a reason for hiding this comment

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

I'd like at least one release where the warnings are elevated to FutureWarning since the warnings aren't visible to users working interactively at the moment

Makes sense to me. Since our next release will be v0.5.0, should we upgrade to FutureWarning in v0.6.0 and then remove in v0.7.0, maybe as part of a "v1 roadmap" to stabilize the public API (xref radiantearth/stac-spec#1184)?

@TomAugspurger
Copy link
Collaborator Author

@gadomski updated to fix the merge conflicts from the signing PR. In general, the ItemSearch consumers call modifier on the "raw" values. The pystac consumer all call the raw consumers internally, so everything should be signed exactly once.

Copy link
Member

@gadomski gadomski left a comment

Choose a reason for hiding this comment

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

Sorry for not catching this earlier, but I think some of the docstrings needed some typing fixups.

@TomAugspurger
Copy link
Collaborator Author

Good catch. Applied those changes and CI is all green.

@gadomski gadomski merged commit ec6762c into stac-utils:main Aug 8, 2022
@TomAugspurger TomAugspurger deleted the fix/item-collection-as-dict-depr branch August 8, 2022 19:07
@gadomski
Copy link
Member

gadomski commented Aug 8, 2022

I've opened #295 to track the FutureWarning upgrade.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

ItemSearch return types overhaul (again) cli.search used deprecated get_all_items_as_dict
4 participants