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

#260: Rename 'Dataset extension' to 'Collection extension' + other improvements #280

Merged
merged 19 commits into from
Oct 13, 2018

Conversation

m-mohr
Copy link
Collaborator

@m-mohr m-mohr commented Oct 10, 2018

As discussed in #260.

  • Rough renaming in texts, samples and code
  • Rename the folder and files and all references
  • Read through all texts and make sure they still make sense
  • Re-check everything after the other parallel PRs are merged

This PR also fixes a bunch of issues in the examples found while reading through them.

@m-mohr m-mohr added the prio: must-have required for release associated with label Oct 10, 2018
@m-mohr m-mohr added this to the 0.6.0-RC1 milestone Oct 10, 2018
@m-mohr m-mohr requested review from cholmes, matthewhanson, hgs-msmith and a user October 11, 2018 12:44
@m-mohr m-mohr changed the title [WIP] #260: Rename 'Dataset extension' to 'Collection extension' #260: Rename 'Dataset extension' to 'Collection extension' Oct 11, 2018
extensions/README.md Outdated Show resolved Hide resolved
Copy link
Contributor

@hgs-msmith hgs-msmith left a comment

Choose a reason for hiding this comment

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

There is a lot here, but it looks correct and complete.
Nice work, Matthias!

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Lots of good cleanup here!

extensions/README.md Outdated Show resolved Hide resolved
Copy link
Collaborator

@matthewhanson matthewhanson left a comment

Choose a reason for hiding this comment

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

@m-mohr I've made several comments

@m-mohr
Copy link
Collaborator Author

m-mohr commented Oct 12, 2018

  • Changed the scope of the sci extension
  • Further restructuring
  • Improved/fixed several links
  • Fixed issues raised here and improved wording regarding relationship between items/collections.
  • Moved and shortened recommendations
  • Added a "Extending STAC" section (based on recommendations)

Everything solved from your side, @matthewhanson?

@m-mohr m-mohr changed the title #260: Rename 'Dataset extension' to 'Collection extension' #260: Rename 'Dataset extension' to 'Collection extension' + other improvements Oct 12, 2018
Recommendations moved elsewhere, so removing this paragraph and its broken link.
The other readme's link to the actual spec in the first couple lines. With this one you have to dig a bit before you get the link, and you could be thrown off by the links to the other specs.
Copy link
Contributor

@cholmes cholmes left a comment

Choose a reason for hiding this comment

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

Overall it looks really great. I put in a number of suggestions. I'm not going to make this 'request changes' so I don't block merging until I wake up on monday. And definitely not everything I wrote is required to do. The things that I would most like to see are:

  • use 'collections' rel in examples instead of 'root', since we are making the former strongly recommended, while the latter we just mention.
  • Extension discussion in the main specs - I'd like to caveat them more, as right now they read like we are fully endorsing extensions at the same level as the main spec. I want us to get there, but I think we need more mature extensions.

I also made a couple minor PR's, so ideally incorporate those.

Thanks for all this work! It's looking really good. Feel free to merge after addressing my points above.

item-spec/examples/sample.json Outdated Show resolved Hide resolved
item-spec/examples/sentinel2-sample.json Show resolved Hide resolved
item-spec/examples/sample-full.json Outdated Show resolved Hide resolved
item-spec/item-spec.md Outdated Show resolved Hide resolved
item-spec/item-spec.md Outdated Show resolved Hide resolved
catalog-spec/catalog-recommendations.md Show resolved Hide resolved
collection-spec/collection-spec.md Show resolved Hide resolved
extensions/scientific/README.md Show resolved Hide resolved
spatial and temporal extents of all the data contained. A STAC search extension would only query those collections which
have data that validates as STAC `Items` - with a datetime field and references to assets. But a STAC can live alongside
spatial and temporal extents of all the data contained. The collections returned are compliant to both WFS Collections and
[STAC collections](../collections-spec/README.md). A STAC search extension would only query those collections which
Copy link
Contributor

Choose a reason for hiding this comment

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

I would ideally like a bit more discussion here, reiterating the point of the STAC collections, and contrasting them against WFS collections, perhaps mentioning any additional alignment we'd see in the future, etc. Not required to merge this by any means, but definitely a nice to have.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Should be added at some point, but don't feel that I can really make a good proposal for this one now. Feel free to make a PR for it?!

@m-mohr m-mohr merged commit cb7cae7 into dev Oct 13, 2018
@m-mohr
Copy link
Collaborator Author

m-mohr commented Oct 13, 2018

I merged this PR now as I think I solved all important comments. Thanks for reviewing. If there's still something missing, I think it is easier to open a separate PR instead of using this rather large and messy PR.

@m-mohr m-mohr deleted the dataset-rename branch October 13, 2018 12:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
prio: must-have required for release associated with
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants