Add collection to ExtensionManagementMixin type params where appropriate by jisantuc · Pull Request #547 · stac-utils/pystac

jisantuc · 2021-07-09T14:20:40Z

Related Issue(s): #525

Description: This PR adds pystac.Collection to the ExtensionManagementMixin type params (via a Union) for all extensions that provide concrete implementations of SummariesExtension.

PR Checklist:

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

It isn't necessarily the case that the extensions in question can be applied to collections (in the sense that their fields mean anything on collection properties) but they _can_ show up in summaries (inferred from th presence of `SummariesFooExtension`).

codecov-commenter · 2021-07-09T14:22:34Z

Codecov Report

Merging #547 (bb42277) into main (073aad2) will increase coverage by 0.05%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main     #547      +/-   ##
==========================================
+ Coverage   94.27%   94.33%   +0.05%     
==========================================
  Files          71       71              
  Lines       10290    10378      +88     
  Branches     1077     1085       +8     
==========================================
+ Hits         9701     9790      +89     
+ Misses        420      419       -1     
  Partials      169      169

Impacted Files	Coverage Δ
pystac/extensions/eo.py	`94.41% <100.00%> (+0.07%)`	⬆️
pystac/extensions/label.py	`93.73% <100.00%> (+0.05%)`	⬆️
pystac/extensions/projection.py	`98.52% <100.00%> (+0.03%)`	⬆️
pystac/extensions/raster.py	`89.85% <100.00%> (+0.47%)`	⬆️
pystac/extensions/sat.py	`100.00% <100.00%> (ø)`
pystac/extensions/scientific.py	`95.09% <100.00%> (+0.09%)`	⬆️
pystac/extensions/timestamps.py	`100.00% <100.00%> (ø)`
pystac/extensions/view.py	`98.42% <100.00%> (+0.03%)`	⬆️
tests/extensions/test_eo.py	`100.00% <100.00%> (ø)`
tests/extensions/test_label.py	`97.15% <100.00%> (+0.07%)`	⬆️
... and 6 more

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 073aad2...bb42277. Read the comment docs.

duckontheweb

This looks great, thanks for getting it in!

Originally I was thinking we should add test cases for using add_to and remove_from on Collection instances to make sure it actually works at runtime, but I'm pretty confident that we are covering our bases with the typing. What do you think @jisantuc?

jisantuc · 2021-07-09T16:08:03Z

How to test was going to be my next question -- conditions are:

runs without crashing
adds the URI appropriately

?

duckontheweb · 2021-07-09T16:57:51Z

How to test was going to be my next question -- conditions are:

runs without crashing

adds the URI appropriately

?

Yeah, and:
3. remove_from removes the URI

jisantuc · 2021-07-09T17:16:50Z

Done in 6fb98fd and b6b2bf8 -- now with a .05% coverage improvement! 🌶

duckontheweb · 2021-07-09T17:23:01Z

Done in 6fb98fd and b6b2bf8 -- now with a .05% coverage improvement! 🌶

Yeehaw!

duckontheweb

Looks good overall.

I'm curious to get other input on how we handle automatically adding schema URIs to objects when summarizing (see review comment for more detail) before merging.

…appropriate

tests should fail

the add_if_missing params happen not to be necessary all over the test file here, but they make the tests resilient to changes in the test data, which I think is nice

…appropriate

duckontheweb

Awesome, thanks for getting this in!

jisantuc added 5 commits July 8, 2021 14:53

Allow eo extension URI in collections

c12612e

ride the struggle 🚌

15c4c8e

use unions instead of type param

dcd8c9a

Don't add collection to eo type var

3c748f7

jisantuc commented Jul 9, 2021

View reviewed changes

Comment thread pystac/extensions/eo.py

Update changelog

15e90e9

jisantuc changed the title ~~Add collectin to ExtensionManagementMixin type params where appropriate~~ Add collection to ExtensionManagementMixin type params where appropriate Jul 9, 2021

duckontheweb approved these changes Jul 9, 2021

View reviewed changes

jisantuc added 2 commits July 9, 2021 11:08

ensure adding summary adds extension schema uri

6fb98fd

ensure removing extension strips schema URI

b6b2bf8

jisantuc requested a review from duckontheweb July 9, 2021 17:41

duckontheweb requested changes Jul 9, 2021

View reviewed changes

Comment thread pystac/extensions/eo.py

Comment thread pystac/extensions/eo.py Outdated

jisantuc added 11 commits July 12, 2021 09:38

move add_to to summaries method

6b4413f

Merge branch 'main' into bugfix/js/add-collection-to-ext-mixin-where-…

5ddab1f

…appropriate

remove all the __init__ overloads for summaries

cc9ce36

tests should fail

update label

8b46817

update projection

f195a53

update raster

4121029

test fixups

be7cdfd

update sat

cc0fe28

update scientific

489d197

update timeestamps

4921190

update view

bb42277

the add_if_missing params happen not to be necessary all over the test file here, but they make the tests resilient to changes in the test data, which I think is nice

jisantuc requested a review from duckontheweb July 12, 2021 16:24

jisantuc requested a review from lossyrob July 12, 2021 16:24

Merge branch 'main' into bugfix/js/add-collection-to-ext-mixin-where-…

fdf5f9d

…appropriate

duckontheweb approved these changes Jul 12, 2021

View reviewed changes

duckontheweb linked an issue Jul 12, 2021 that may be closed by this pull request

EO extension can't be added to Collections #525

Closed

duckontheweb added this to the 1.0.0 milestone Jul 12, 2021

duckontheweb merged commit f443f6a into stac-utils:main Jul 12, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add collection to ExtensionManagementMixin type params where appropriate#547

Add collection to ExtensionManagementMixin type params where appropriate#547
duckontheweb merged 20 commits into
stac-utils:mainfrom
jisantuc:bugfix/js/add-collection-to-ext-mixin-where-appropriate

jisantuc commented Jul 9, 2021 •

edited

Loading

Uh oh!

codecov-commenter commented Jul 9, 2021 •

edited

Loading

Uh oh!

Uh oh!

duckontheweb left a comment

Uh oh!

jisantuc commented Jul 9, 2021

Uh oh!

duckontheweb commented Jul 9, 2021

Uh oh!

jisantuc commented Jul 9, 2021

Uh oh!

duckontheweb commented Jul 9, 2021

Uh oh!

duckontheweb left a comment

Uh oh!

Uh oh!

Uh oh!

duckontheweb left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Conversation

jisantuc commented Jul 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov-commenter commented Jul 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

duckontheweb left a comment

Choose a reason for hiding this comment

Uh oh!

jisantuc commented Jul 9, 2021

Uh oh!

duckontheweb commented Jul 9, 2021

Uh oh!

jisantuc commented Jul 9, 2021

Uh oh!

duckontheweb commented Jul 9, 2021

Uh oh!

duckontheweb left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

duckontheweb left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

jisantuc commented Jul 9, 2021 •

edited

Loading

codecov-commenter commented Jul 9, 2021 •

edited

Loading