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

Structure and cleanup #258

Merged
merged 15 commits into from
Oct 8, 2018
Merged

Structure and cleanup #258

merged 15 commits into from
Oct 8, 2018

Conversation

cholmes
Copy link
Contributor

@cholmes cholmes commented Oct 2, 2018

Large PR that focuses on the overall structure and flow of the spec, aligning with all the great new changes, and doing lots of different clean up.

In particular, it focuses on #195 #194 and #152

It is marked Work In Progress as there is more to do, but making a PR so people can see what is coming.

@m-mohr
Copy link
Collaborator

m-mohr commented Oct 3, 2018

I think this PR should point to dev and not master?

@cholmes cholmes changed the base branch from master to dev October 3, 2018 17:49
@cholmes
Copy link
Contributor Author

cholmes commented Oct 3, 2018

Ah, yes. Changed, thanks!


```

This could also extend to product level metadata:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is it 'product' level? Overall, the following example seems confusing and outdated... bands vs eo:bands, other eo fields etc.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't get a chance to look in depth at everything that was in this document. It's in the repo now, so was just trying to make it a bit better. If there's not time to really go over it then we can just remove it. Best thing to do might be to move this particular discussion to an issue, since there's not really a great 'recommendation' there. I just wanted to make sure this bit wasn't lost, as it captured some of the previous ideas / options.

@@ -61,7 +142,7 @@ of canonical product definitions online, that all static catalogs using a partic
able to reference.


## Product
### Product
Copy link
Collaborator

Choose a reason for hiding this comment

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

Products are outdated => Dataset

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well I think product was more just supposed to capture the asset level metadata, not the dataset level stuff. I agree that it should be added at the dataset 'level', but we don't have the clear proposal for how to do that. I'm not sure if we should require binding it to the dataset level - at Planet we have a lot of assets and it may make sense for an Item's asset to refer to its 'product definition' (asset metadata) dynamically, instead of saying it needs to be enumerated at the dataset level.

Though not sure if this is the best place to discuss this - this PR is just an attempt to make various bits better...

cholmes and others added 10 commits October 4, 2018 18:15
Mostly just removed outdated ideas
Tweaked the first bits, and added more on evolution and alignment with other specs.
Tweaked the readme to help explain a bit more.
* Made intro sentence describe what it does instead of what it extends
* fixed link.
Would like to refine these a bit more, as the explanations aren't great (could use examples, etc) but wanted to at least get the basics in.
@cholmes cholmes changed the title [WIP] Structure and cleanup Structure and cleanup Oct 7, 2018
@cholmes
Copy link
Contributor Author

cholmes commented Oct 7, 2018

Ok, this is good for review.

1 similar comment
@cholmes
Copy link
Contributor Author

cholmes commented Oct 7, 2018

Ok, this is good for review.

Copy link
Collaborator

@m-mohr m-mohr left a comment

Choose a reason for hiding this comment

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

Looks good to me.

@matthewhanson matthewhanson self-requested a review October 8, 2018 15:39
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.

3 participants