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

Add support for selecting the search endpoint using media_type #142

Merged
merged 12 commits into from
Apr 19, 2022

Conversation

mneagul
Copy link
Contributor

@mneagul mneagul commented Feb 24, 2022

Related Issue(s): #

stac-utils/pystac#704

Description:

Added support for selecting the search endpoint using the media_type argument to get_single_link.
This should fix interoperability STAC implementations, particularly with the STAC extension in GeoServer.

PR Checklist:

  • Code is formatted
  • Tests pass
  • Changes are added to the CHANGELOG

@mneagul
Copy link
Contributor Author

mneagul commented Feb 24, 2022

The dependencies should also be updated but I'm not sure how to proceed, what is the policy regarding this.

Copy link
Collaborator

@duckontheweb duckontheweb left a comment

Choose a reason for hiding this comment

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

Thanks for putting this in @mneagul.

See comments below on hard-coding the media_type argument rather than passing it through the containing methods. Let me know if I'm missing a use-case for changing that value per API.

We should also update the PySTAC requirement to pystac~=1.4 to ensure we have a version of PySTAC that supports the media_type argument in Catalog.get_links and Catalog.get_single_link.

pystac_client/client.py Outdated Show resolved Hide resolved
pystac_client/client.py Outdated Show resolved Hide resolved
pystac_client/client.py Outdated Show resolved Hide resolved
pystac_client/client.py Outdated Show resolved Hide resolved
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.

@duckontheweb you suggested pystac~=1.4 but (per previous dependency discussions ala stac-utils/stactools#228) should we do pystac >= 1.4? I could see the case for ~= since we control PySTAC -- curious your and others' thoughts.

CHANGELOG.md Outdated Show resolved Hide resolved
mneagul and others added 6 commits March 1, 2022 20:04
Co-authored-by: Jon Duckworth <duckontheweb@gmail.com>
Co-authored-by: Jon Duckworth <duckontheweb@gmail.com>
Co-authored-by: Jon Duckworth <duckontheweb@gmail.com>
Co-authored-by: Jon Duckworth <duckontheweb@gmail.com>
Co-authored-by: Pete Gadomski <pete.gadomski@gmail.com>
@mneagul
Copy link
Contributor Author

mneagul commented Apr 5, 2022

Hi Everyone,

How should I proceed with this PR ? The tests fail due to dependency issues.

Marian

@duckontheweb
Copy link
Collaborator

@duckontheweb you suggested pystac~=1.4 but (per previous dependency discussions ala stac-utils/stactools#228) should we do pystac >= 1.4? I could see the case for ~= since we control PySTAC -- curious your and others' thoughts.

Sorry I dropped the ball on this one...

I don't have really strong opinions on this, but I tend to opt for ~=X.x in most cases to allow for feature updates but not major version updates. Since major version updates are likely to have breaking changes that could break our code, I typically like to stop short of allowing those. I can see the argument for not restricting the upper bounds, though, and just waiting to see if it's actually a problem, so I'm fine with whatever the consensus is.

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.

#144 will fix the PySTAC dependency issue, so once that gets merged this PR shouldn't need to worry about the dependency stuff.

CHANGELOG.md Outdated Show resolved Hide resolved
@duckontheweb duckontheweb mentioned this pull request Apr 5, 2022
3 tasks
Co-authored-by: Pete Gadomski <pete.gadomski@gmail.com>
@duckontheweb
Copy link
Collaborator

Since this PR is introducing a change that requires PySTAC>=1.4.0, maybe it would actually be best to update that dependency here rather than in a separate PR.

@gadomski @matthewhanson If you agree then I think the desired change would be to update the PySTAC dependency to pystac>=1.4.0.

@gadomski gadomski mentioned this pull request Apr 13, 2022
3 tasks
pystac_client/client.py Outdated Show resolved Hide resolved
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.

This branch needs to merge main to pick up the latest changes -- I don't have permission to push to the source branch so can't do it myself.

pystac_client/client.py Outdated Show resolved Hide resolved
@mneagul
Copy link
Contributor Author

mneagul commented Apr 19, 2022

I updated merged the latest changes. I hope everything is ok.

@codecov-commenter
Copy link

codecov-commenter commented Apr 19, 2022

Codecov Report

Merging #142 (6678ca5) into main (4eaab55) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main     #142   +/-   ##
=======================================
  Coverage   80.93%   80.93%           
=======================================
  Files           9        9           
  Lines         556      556           
=======================================
  Hits          450      450           
  Misses        106      106           
Impacted Files Coverage Δ
pystac_client/client.py 77.96% <100.00%> (ø)

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 4eaab55...6678ca5. Read the comment docs.

@gadomski gadomski merged commit b8ff817 into stac-utils:main Apr 19, 2022
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.

4 participants