Skip to content

Implement Extension: Render (#1464) #1465

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

Merged
merged 18 commits into from
Jan 18, 2025

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.43%. Comparing base (1cf7f3f) to head (d5e4cd4).
Report is 34 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1465      +/-   ##
==========================================
+ Coverage   91.23%   91.43%   +0.20%     
==========================================
  Files          53       54       +1     
  Lines        7335     7498     +163     
  Branches      894      906      +12     
==========================================
+ Hits         6692     6856     +164     
  Misses        457      457              
+ Partials      186      185       -1     

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

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@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.

@gadomski gadomski removed their request for review December 18, 2024 14:20
@bmcandr bmcandr marked this pull request as ready for review January 16, 2025 19:36
@gadomski
Copy link
Member

@bmcandr is this ready for another round of review? Thanks for the sweat-equity on this!

@bmcandr
Copy link
Contributor Author

bmcandr commented Jan 18, 2025

Yep!

@gadomski gadomski self-requested a review January 18, 2025 02:37
Copy link
Member

@gadomski gadomski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Excellent stuff...thanks!

@gadomski gadomski added this pull request to the merge queue Jan 18, 2025
Merged via the queue into stac-utils:main with commit 312a081 Jan 18, 2025
23 checks passed
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