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

initial text for Label changes #1001

Closed
wants to merge 3 commits into from

Conversation

philvarner
Copy link
Collaborator

Related Issue(s): #936

Proposed Changes:

  1. in Label Extension, use Asset Role instead of Asset name to indicate the labels assets
  2. refine language around raster vs. vector labels

PR Checklist:

  • This PR is made against the dev branch (all proposed changes except releases should be against dev, not master).
  • This PR has no breaking changes.
  • I have added my changes to the CHANGELOG or a CHANGELOG entry is not required.
  • This PR affects the STAC API spec, and I have opened issue/PR #XXX to track the change.

@philvarner philvarner requested a review from jisantuc February 22, 2021 15:39
@cholmes cholmes mentioned this pull request Feb 23, 2021
3 tasks
@cholmes cholmes added this to the 1.0.0-RC.1 milestone Feb 24, 2021
@cholmes cholmes linked an issue Feb 25, 2021 that may be closed by this pull request
Copy link
Contributor

@jisantuc jisantuc left a comment

Choose a reason for hiding this comment

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

woops I made these comments the other day but forgot to submit the review. Anyway this looks pretty sensible to me -- I'm open to being convinced about both comments.

extensions/label/README.md Outdated Show resolved Hide resolved
Comment on lines +143 to +146
##### Asset names

The Label Extension recommends assets be named with the keys "raster_labels" or "vector_labels". However, this is just a recommendation, and acutal name are entirely at the discretion of the implementer.

Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
##### Asset names
The Label Extension recommends assets be named with the keys "raster_labels" or "vector_labels". However, this is just a recommendation, and acutal name are entirely at the discretion of the implementer.

kind of the inverse of the previous comment -- I think tooling developers would be better off if there weren't recommended names for things, since the lines between recommended, required, and "required for this one tool to work" aren't always clearly drawn.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I agree. I only left this in the spec b/c it was the required way to indicate labels previously. I'm fine with removing it, but I'm worried that their might be tools that rely on it? But, maybe better just to remove it now before 1.0.0?

@philvarner philvarner marked this pull request as ready for review March 3, 2021 15:37
@philvarner philvarner closed this Mar 3, 2021
@philvarner philvarner deleted the label-asset-role branch March 3, 2021 18:33
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.

Label Extension: Use asset role "labels" to
4 participants