Add a post-init modifier keyword

[TomAugspurger]

Related Issue(s):

• Closes Feature request: Support "signing" results #259

Description:

This adds a new keyword, modifier, to the various constructors. The motivation is described in #259. As a brief demonstration:

In [1]: import planetary_computer, pystac_client

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

In [3]: item = next(catalog.get_collection("sentinel-2-l2a").get_all_items())

In [4]: item.assets["B02"].href  # signed URL, notice the querystring
Out[4]: 'https://sentinel2l2a01.blob.core.windows.net/sentinel2-l2/52/S/GH/2022/07/28/S2B_MSIL2A_20220728T020659_N0400_R103_T52SGH_20220729T082039.SAFE/GRANULE/L2A_T52SGH_A028157_20220728T021310/IMG_DATA/R10m/T52SGH_20220728T020659_B02_10m.tif?st=2022-07-28T16%3A03%3A54Z&se=2022-07-29T16%3A48%3A54Z...'

This is kind of a large PR, partly from overriding the __init__ methods so that we can set this new attribute. This also seemed to cause several issues with mypy, some of which I'm stumped by.

But the core of the change is to

1. Call self.modifier(thing) before returning anything to the user.
2. Pass through self.modifier to child collections / catalogs.

---

A couple notes / collections

1. I didn't copy over the class docstrings for Catalog and Collection or any of the from_dict / from_file methods from pystac so those won't directly mention modifier anywhere, despite being a valid keyword. I can either copy them over, override them with a short description of modifier and then point to the parent, or just leave it as is.
2. A couple of the code paths aren't covered by my new test. I'll point those out inline.

PR Checklist:

• Code is formatted
• Tests pass
• Changes are added to the [CHANGELOG](https://github.com/stac-utils/pystac-api-client/blob/main/CHANGELOG.md

[TomAugspurger]

One concern I had with expecting modifier to mutate in-place: PC users will be used to planteray_computer.sign, which copies. So doing Catalog.open(..., modifier=planetary_computer.sign) will silently not work. We can document this, but I'm a bit worried about it.

I wonder if our calls to self.modifier should check that the return type is None and emit a warning if it isn't?

result = self.modifier(thing)
if result is not None:
    warnings.warn("Modifier returned a value...")

That'd be a "false positive" if you have a function that mutates and returns a value. But maybe that's OK?

[codecov-commenter]

Codecov Report

Merging #286 (23f2e10) into main (082c5f0) will increase coverage by 0.82%.
The diff coverage is 81.25%.

@@            Coverage Diff             @@
##             main     #286      +/-   ##
==========================================
+ Coverage   83.54%   84.36%   +0.82%     
==========================================
  Files          10       11       +1     
  Lines         717      774      +57     
==========================================
+ Hits          599      653      +54     
- Misses        118      121       +3     

Impacted Files	Coverage Δ
pystac_client/client.py	84.84% <73.07%> (+3.36%)	⬆️
pystac_client/collection_client.py	81.96% <80.00%> (+5.22%)	⬆️
pystac_client/item_search.py	91.61% <82.35%> (-0.39%)	⬇️
pystac_client/_utils.py	100.00% <100.00%> (ø)
pystac_client/errors.py	100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us.

[TomAugspurger]

3ca725e implemented a check similar to #286 (comment).

After we call modifier, we check that the result is either None or is the same python object. If it's a new object then we emit the warning. I think a warning is more appropriate than an error, but I could do either here.

[gadomski]

Makes sense to me. The only design decision I disagreed with was pystac_client._utils.no_modifier; Optional[Callable[[Modifiable], None]] is clearer to me. But that's a six/half-dozen decision so I'm happy with how things are as-is.

Thanks for the tests, it feels very easy to miss a branch and not pass through/modify correctly.

[gadomski]

Whoops forgot that I requested changes inline 🤦🏽

[gadomski]

@philvarner @duckontheweb I've added both of you as reviewers; not required, but if you do have a spare moment to take a look, this PR touches a lot of code and it wouldn't hurt to have a second set of eyes.

[TomAugspurger]

Makes sense to me. The only design decision I disagreed with was pystac_client._utils.no_modifier; Optional[Callable[[Modifiable], None]] is clearer to me.

I'm fine with this too, if you also prefer it. It saves a function call, so we might as well.

[duckontheweb]

Thanks @TomAugspurger, I think this is going to end up being really helpful even outside of the Planetary Computer use-case.

I left one fix to remove a # type: ignore comment in one of the tests, but other than that it looks good to me.

[TomAugspurger]

Thanks all!