Skip to content

Conversation

@duckontheweb
Copy link
Collaborator

@duckontheweb duckontheweb commented Nov 30, 2021

Related Issue(s):

Description:

Ensures that query string parameters and headers given in Client.open or Client.from_file are used in requests from CollectionClient instances fetched from that API.

PR Checklist:

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

@codecov-commenter
Copy link

codecov-commenter commented Nov 30, 2021

Codecov Report

Merging #126 (bb7d6d9) into main (883316b) will not change coverage.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #126   +/-   ##
=======================================
  Coverage   79.59%   79.59%           
=======================================
  Files           9        9           
  Lines         549      549           
=======================================
  Hits          437      437           
  Misses        112      112           
Impacted Files Coverage Δ
pystac_client/client.py 77.96% <100.00%> (ø)
pystac_client/collection_client.py 85.71% <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 883316b...bb7d6d9. Read the comment docs.

@duckontheweb duckontheweb changed the title Fix/propagate root io Propagate root StacAPIIO to ItemSearch in Client.get_items Nov 30, 2021
link = self.get_single_link('items')
if link is not None:
search = ItemSearch(link.href, method='GET')
search = ItemSearch(link.href, method='GET', stac_io=self.get_root()._stac_io)
Copy link
Member

Choose a reason for hiding this comment

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

Nothing technically wrong here, but accessing a "private" member of an object retrieved via a function call feels like we're breaking abstraction. This isn't surprising given the nature of how the StacIO stuff has to work, but I'd just flag this for a bit of code smell w/ maybe the chance to refactor sometime down the line. No changes required now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah, thanks for flagging that. I had originally added a "public" stac_io property to pystac.Catalog in stac-utils/pystac#590, but we ultimately decided that it wasn't needed as part of that PR, so I took it back out. Given that there are a couple of places where it is accessed in the pystac-client code, it might be worth revisiting that.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Opened stac-utils/pystac#675 to discuss adding a "public" attribute/property.

@matthewhanson matthewhanson merged commit 3d2a2e8 into stac-utils:main Jan 4, 2022
@duckontheweb duckontheweb deleted the fix/propagate-root-io branch January 19, 2022 17:48
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.

StacApiIO instance not propagated to CollectionClient instances

4 participants