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

Add flatgeobuf media type #938

Merged

Conversation

pjhartzell
Copy link
Collaborator

@pjhartzell pjhartzell commented Jan 4, 2023

Related Issue(s):
#763

Description:
Adds a media type for flatgeobuf data.

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.

@codecov-commenter
Copy link

codecov-commenter commented Jan 4, 2023

Codecov Report

Base: 94.37% // Head: 94.37% // Increases project coverage by +0.00% 🎉

Coverage data is based on head (6e1424d) compared to base (1bdd09b).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #938   +/-   ##
=======================================
  Coverage   94.37%   94.37%           
=======================================
  Files          83       83           
  Lines       12015    12016    +1     
  Branches     1136     1136           
=======================================
+ Hits        11339    11340    +1     
  Misses        496      496           
  Partials      180      180           
Impacted Files Coverage Δ
pystac/media_type.py 100.00% <100.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@gadomski gadomski self-requested a review January 5, 2023 20:58
@gadomski gadomski added this to the 1.7 milestone Jan 5, 2023
@gadomski gadomski linked an issue Jan 5, 2023 that may be closed by this pull request
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.

Per flatgeobuf/flatgeobuf#112 (comment), it seems like there's still an open question of the "correct" media type -- @m-mohr's suggestion in that comment is application/vnd.flatgeobuf, but this PR uses application/flatgeobuf. I'm hesitant to approve w/o input form @m-mohr or someone else who might have guidance on the best one to pick for PySTAC.

@gadomski gadomski requested a review from m-mohr January 5, 2023 21:34
@m-mohr
Copy link
Contributor

m-mohr commented Jan 5, 2023

I think this must be approved by the official maintainer of flatgeobuf, all other attempts from "unofficial" side just leads to issues later on, like with GeoTiff/COG where the whole community uses what we invented for STAC, but in the end we figured out that this can't be the official media type from OGC (or IANA).

@gadomski gadomski removed the request for review from m-mohr January 5, 2023 21:42
@gadomski
Copy link
Member

gadomski commented Jan 5, 2023

I've asked for guidance from the flatgeobuf folks at flatgeobuf/flatgeobuf#112 (reply in thread). Marking this PR as a draft until that question is settled.

@gadomski gadomski marked this pull request as draft January 5, 2023 21:47
@gadomski gadomski removed this from the 1.7 milestone Jan 5, 2023
@gadomski
Copy link
Member

gadomski commented Jan 6, 2023

@pjhartzell per flatgeobuf/flatgeobuf#112 (reply in thread), can you update to application/vnd.flatgeobuf, and add a comment linking to that discussion?

@pjhartzell pjhartzell force-pushed the issues/763-flatgeobuf-media-type branch from ac2d617 to 7028027 Compare January 6, 2023 14:38
@pjhartzell
Copy link
Collaborator Author

@gadomski Updated.

@pjhartzell
Copy link
Collaborator Author

Are we good with the enum name FGB? It was either that or FLATGEOBUF.

pystac/media_type.py Outdated Show resolved Hide resolved
@pjhartzell pjhartzell force-pushed the issues/763-flatgeobuf-media-type branch from 7028027 to 7bd822a Compare January 6, 2023 14:52
@pjhartzell pjhartzell requested a review from gadomski January 6, 2023 14:53
@gadomski
Copy link
Member

gadomski commented Jan 6, 2023

flatgeobuf/flatgeobuf#112 (reply in thread) feels like Good Enough™ to me -- marking this as ready to review.

@gadomski gadomski marked this pull request as ready for review January 6, 2023 18:47
@pjhartzell
Copy link
Collaborator Author

@gadomski Discussion link now in comment and changelog. 🙂

@gadomski gadomski merged commit d28bd43 into stac-utils:main Jan 6, 2023
@gadomski gadomski added this to the 1.7 milestone Jan 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support FlatGeobuf as media_type
4 participants