-
Notifications
You must be signed in to change notification settings - Fork 179
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
[Sat Extension] Additional acquisition parameters #894
Conversation
new items describing acquisition parameters
schema + CHANGELOG
Hello. Can we move on this PR? @m-mohr Do the reviews need more clarifications? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall I think this is looking good. I made a couple comments, but I think the overall thing is just add a bit more 'why' to the descriptions, help people understand what these are used for. But thanks for the contribution, it's great to see these suggestions come from working with the spec and data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See comments :-)
Co-authored-by: Chris Holmes <chomie@gmail.com>
Co-authored-by: Matthias Mohr <webmaster@mamo-net.de>
Co-authored-by: Matthias Mohr <webmaster@mamo-net.de>
Co-authored-by: Matthias Mohr <webmaster@mamo-net.de>
There was a problem hiding this 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, with one minor english tweak. Thanks!
Co-authored-by: Chris Holmes <chomie@gmail.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So it's looks good to be merged in the spec. I'm personally neutral on these changes, but I need to approve after I have requested changes so I'll approve to unblock the merge. Nevertheless, I'd be happy if more data providers could look over the changes and approve them. So I'm not merging myself, although the required number of 2 reviewers have been reached.
Ok, so how do we proceed? Is there something we can do? |
@emmanuelmathot Maybe get another "independent" voice that confirms that these changes make sense to them as well and/or they use these fields? |
I'm fine to merge this. If it was in core I'd want more independent review, but I'm ok in an extension. I think it's good to provide more optional fields, instead of making everyone come up with their own. If one of the field names ends up totally wrong we can deprecate it and add a new one, without breaking backwards compatability. |
Proposed Changes:
PR Checklist: