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

Implement Extension: Render (#1464) #1465

Draft
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

bmcandr
Copy link
Contributor

@bmcandr bmcandr commented Oct 25, 2024

Related Issue(s):

Description:
Implements the Render extension.

@gadomski

PR Checklist:

  • Pre-commit hooks and 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.

@bmcandr
Copy link
Contributor Author

bmcandr commented Oct 25, 2024

I need to add docs, tests, update changelog, etc. but figured I'd get the PR open since there may be other changes I need to make. I'll be able to do this next week.

As I mentioned on Gitter, I think the published schema for v1.0.0 is not correct because Items (including the examples in the extension spec repo) fail to validate.

Copy link

codecov bot commented Oct 25, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.36%. Comparing base (b052b2a) to head (2d65be1).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1465      +/-   ##
==========================================
+ Coverage   91.15%   91.36%   +0.20%     
==========================================
  Files          52       53       +1     
  Lines        7251     7414     +163     
  Branches      875      887      +12     
==========================================
+ Hits         6610     6774     +164     
  Misses        461      461              
+ Partials      180      179       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@gadomski gadomski marked this pull request as draft October 25, 2024 20:28
@gadomski gadomski self-requested a review October 25, 2024 20:28
@bmcandr
Copy link
Contributor Author

bmcandr commented Oct 30, 2024

I think I figured out why example Items on main in the spec repo and Items produced by my implementation fail validation. The v1.0.0 spec defines renders as a required field on Items, but an unreleased update moves renders field into the Item properties to align with the STAC spec convention for additional metadata. My implementation of the extension follows this convention reflected in the unreleased spec and AFAICT all Item-extending extensions implemented in pystac at this time follow this convention too. IMO it makes sense to wait for the updated spec to be released before considering this extension for acceptance.

Edit: the spec author is considering reverting this change to make renders a top-level field on Items. Does this jive with how extensions are currently handled in pystac? Are there any Item-extending extension implementations that place additional metadata at the top-level?

Edit 2: the spec author has agreed to stick with convention 🎉

@gadomski
Copy link
Member

Edit 2: the spec author has agreed to stick with convention 🎉

Awesome, thanks for the update. Yeah, everything should be on properties so glad you sorted that out.

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.

2 participants