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

Collapse eventstream with the streaming trait #365

Merged
merged 4 commits into from
Apr 10, 2020

Conversation

JordonPhillips
Copy link
Contributor

The eventstream and streaming trait fundamentally work in similar
ways, and have identical validation rules. This merges the two traits
to simplify the model and reduce duplication.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

The eventstream and streaming trait fundamentally work in similar
ways, and have identical validation rules. This merges the two traits
to simplify the model and reduce duplication.
Copy link
Member

@mtdowling mtdowling left a comment

Choose a reason for hiding this comment

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

Seems like "Initial messages" and "Initial-request" should go under the "Event streams" section. Can "Initial message client and server behavior" be shortened to "Initial message behavior"?

docs/source/spec/core/stream-traits.rst Outdated Show resolved Hide resolved
docs/source/spec/core/stream-traits.rst Outdated Show resolved Hide resolved
docs/source/spec/core/stream-traits.rst Outdated Show resolved Hide resolved
docs/source/spec/core/stream-traits.rst Outdated Show resolved Hide resolved
- Description
* - requiresLength
- ``boolean``
- Indicates that the stream must have a known size.
Copy link
Member

Choose a reason for hiding this comment

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

This can only be set when attached to blobs, right? Should this maybe be a separate trait to make this discrepancy less weird?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe. Is there not a use case for it for event streams?

Copy link
Member

@mtdowling mtdowling Apr 10, 2020

Choose a reason for hiding this comment

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

I doubt it. Event streams usually don't have a predefined size or bounds.

docs/source/spec/core/stream-traits.rst Outdated Show resolved Hide resolved
docs/source/spec/core/stream-traits.rst Outdated Show resolved Hide resolved
docs/source/spec/core/stream-traits.rst Outdated Show resolved Hide resolved
docs/source/spec/core/stream-traits.rst Show resolved Hide resolved
docs/source/spec/core/stream-traits.rst Show resolved Hide resolved
Copy link
Member

@mtdowling mtdowling left a comment

Choose a reason for hiding this comment

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

I think requiresLength is still odd since it doesn't make sense on eventStreams, but we can followup on that

@JordonPhillips JordonPhillips merged commit 6c81c9b into smithy-lang:0.10 Apr 10, 2020
@kstich kstich mentioned this pull request Apr 23, 2020
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.

2 participants