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

added type property for catalogs and collections #288

Merged
merged 2 commits into from
Apr 28, 2021
Merged

added type property for catalogs and collections #288

merged 2 commits into from
Apr 28, 2021

Conversation

volaya
Copy link
Contributor

@volaya volaya commented Mar 17, 2021

Related Issue(s): #
radiantearth/stac-spec#971

Description:
Adds tpe property for catalogs and collections

PR Checklist:

  • Code is formatted (run scripts/format)
  • Tests pass (run scripts/test)
  • 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.

@codecov-io
Copy link

Codecov Report

Merging #288 (1d2b7c1) into develop (5a492ca) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff            @@
##           develop     #288   +/-   ##
========================================
  Coverage    93.68%   93.68%           
========================================
  Files           33       33           
  Lines         4057     4057           
========================================
  Hits          3801     3801           
  Misses         256      256           
Impacted Files Coverage Δ
pystac/catalog.py 95.67% <ø> (ø)

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 5a492ca...1d2b7c1. Read the comment docs.

@volaya
Copy link
Contributor Author

volaya commented Mar 17, 2021

Before merging this, one thing to consider:

Pystac uses the 'extent' property to identify collections (see https://github.com/stac-utils/pystac/blob/develop/pystac/serialization/identify.py#L273). That should be changed to now use the type property, but doing so breaks a large number of tests, mainly because sample json files in tests do not have the type property. Those files should be updated, but before working on that, I would like a second opinion about it, to know if it is worth or not. For what I understand, the type property is now mandatory, so sample test catalogs and collections should have it.

@matthewhanson
Copy link
Member

@volaya That's correct, this is a required property now, and Pystac should be updated to use the type to identify Collections. I think all the tests should be updated as well. I think I'd combine the identify changes and example changes into this PR.

@lossyrob
Copy link
Member

The identify functionality identifies older version of STAC as well, so we don't want to get rid of that. It can start with a check for the property type and make decisions based on that, but if something is pre-1.0.0-RC1, it will need to fall back to it's current logic.

The STAC objects that should be updated are the main test data, but all the examples test data that are for a specific version shouldn't be changed - those test PySTAC's ability to migrate older versions to new STAC.
r

@volaya
Copy link
Contributor Author

volaya commented Mar 24, 2021

I added a new commit with changes to the identify function, which now uses the approach suggested by @lossyrob (first try to identify based on the type property, and if that's not possible, use the previous approach).

This also fixes a problem not mentioned in #289 With that PR, Collections can now have assets, and the identify method uses the presence of the 'assets' property to identify Item objects, which will no longer be a valid strategy. No need to change anything in that PR if this one is merged.

@lossyrob lossyrob merged commit b75f32e into stac-utils:main Apr 28, 2021
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