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

Removing transmitter supplied fields from stream config PUT and PATCH… #152

Merged
merged 2 commits into from
May 10, 2024

Conversation

ysarig1
Copy link
Collaborator

@ysarig1 ysarig1 commented Apr 17, 2024

Removed the transmitter supplied fields from the stream config PUT and PATCH examples.

@ysarig1 ysarig1 requested review from FragLegs, appsdesh and tulshi April 17, 2024 00:27
@ysarig1
Copy link
Collaborator Author

ysarig1 commented Apr 22, 2024

@FragLegs, @appsdesh, @atultulshi Can you please review the PR and approve/comment?

Copy link
Contributor

@appsdesh appsdesh left a comment

Choose a reason for hiding this comment

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

The existing language already clarifies -

Transmitter-Supplied properties beside the stream_id MAY be present,
but they MUST match the expected value. Missing Transmitter-Supplied
properties MUST be ignored by the Transmitter.

There may be reasons for the receiver to send the same values to make sure it's info matches the Tx info (and expect to receive error message, on mismatch)

I don't think this PR is needed. Please let me know your thoughts

@ysarig1
Copy link
Collaborator Author

ysarig1 commented Apr 23, 2024

The existing language already clarifies -

Transmitter-Supplied properties beside the stream_id MAY be present,
but they MUST match the expected value. Missing Transmitter-Supplied
properties MUST be ignored by the Transmitter.

There may be reasons for the receiver to send the same values to make sure it's info matches the Tx info (and expect to receive error message, on mismatch)

I don't think this PR is needed. Please let me know your thoughts

The issue addressed by this PR is not if the fields can or can't be in the request but the fact that having a the fields in the examples is confusing. Having transmitter-supplied fields in an update request is an edge case and while it is allowed, it doesn't make sense to do so.

Copy link
Contributor

@FragLegs FragLegs left a comment

Choose a reason for hiding this comment

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

Thanks for working on this!

@ysarig1 ysarig1 requested a review from a team as a code owner May 10, 2024 23:46
@tulshi tulshi dismissed appsdesh’s stale review May 10, 2024 23:48

Examples without the transmitter supplied fields are clearer, so this is a net benefit to the spec.

@tulshi tulshi merged commit 92785dd into main May 10, 2024
2 checks passed
@tulshi tulshi deleted the issue-150 branch May 10, 2024 23:48
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.

The examples for stream update requests should not include Transmitter-Supplied fields in the request
4 participants