pystac icon indicating copy to clipboard operation
pystac copied to clipboard

Add an AsIsLayoutStrategy

Open gadomski opened this issue 3 years ago • 2 comments

Related Issue(s):

  • Helps fix #875

Description:

Currently, it's impossible to use Catalog.add_item but preserve the self_href of the item. This PR adds a new layout that simply preserves the existing href, or raises a ValueError if one isn't set.

I don't love the name AsIsLayoutStrategy, so improving suggestions are welcome! Other names I considered:

  • NoopLayoutStrategy
  • PassthroughLayoutStrategy

cc @kylemann16

PR Checklist:

  • [x] Code is formatted (run pre-commit run --all-files)
  • [x] Tests pass (run scripts/test)
  • [x] Documentation has been updated to reflect changes, if applicable
  • [x] This PR maintains or improves overall codebase code coverage.
  • [x] Changes are added to the CHANGELOG. See the docs for information about adding to the changelog.

gadomski avatar Nov 10 '22 22:11 gadomski

Codecov Report

Base: 90.12% // Head: 90.08% // Decreases project coverage by -0.04% :warning:

Coverage data is based on head (01b9467) compared to base (f44de3c). Patch coverage: 62.50% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #919      +/-   ##
==========================================
- Coverage   90.12%   90.08%   -0.04%     
==========================================
  Files          47       47              
  Lines        6063     6079      +16     
  Branches      909      912       +3     
==========================================
+ Hits         5464     5476      +12     
- Misses        420      422       +2     
- Partials      179      181       +2     
Impacted Files Coverage Δ
pystac/layout.py 87.86% <62.50%> (-2.14%) :arrow_down:
pystac/stac_object.py 91.89% <0.00%> (+1.08%) :arrow_up:

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

:umbrella: View full report at Codecov.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

codecov-commenter avatar Nov 15 '22 03:11 codecov-commenter

@gadomski Sorry for the late response here, but thanks for looking at this. Those names look good to me, maybe other options would beExplicitUrlLayoutStrategy, UrlNoopLayoutStrategy, or something to indicate that the user is handling all links themselves?

kylemann16 avatar Nov 15 '22 17:11 kylemann16

UrlNoopLayoutStrategy

Yeah, I thought about NoopLayoutStrategy as well -- but then thought that it looked funny :-). I don't really have a strong inclination one way or the other, so will leave it to a reviewer to suggest/approve one of these (or other) options.

gadomski avatar Jan 23 '23 16:01 gadomski

Side note: The add_item() method on the Catalog class indicates the strategy parameter is optional, but it really is required with a default of BestPracticesLayoutStrategy (see the if strategy is None: line). Also, the strategy parameter is not given in the docstring Args list, adding another layer of ambiguity here.

https://github.com/stac-utils/pystac/blob/ea68043eaf579222fe8985f73e74aaf58e5943e7/pystac/catalog.py#L277-L297

Also, it seems logical for a strategy parameter to be added to the Catalog.add_items() method.

Similar thoughts apply to the Collection class add_item() method.

pjhartzell avatar Jan 25 '23 14:01 pjhartzell

I opened https://github.com/stac-utils/pystac/issues/965 to capture the need to align the function signatures.

gadomski avatar Jan 25 '23 16:01 gadomski