-
Notifications
You must be signed in to change notification settings - Fork 10
Setup connection simplest spec #50
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
Setup connection simplest spec #50
Conversation
whyoleg
left a comment
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.
minor kotlin style related things
| val payload: Payload? | ||
| ) : Frame<SetupFlags>(FrameType.SETUP) { | ||
|
|
||
| constructor( |
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.
null can be used as default parameter in primary constructor
| writeByte(dataMimeType.length.toInt()) | ||
| writeUtf8(dataMimeType.text) | ||
| payload.metadata?.length?.let(this::writeLength) | ||
| if (metadataMimeType != null) { |
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.
As I remember mime types are required
so, need to write at least one byte with 0 value
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.
@OlegDokuka I've not found if mime type is required or not in RSocket spec.
My question is should spec says explicitly is mime type required during connection establishment?
tck-spec/build.gradle
Outdated
| implementation("org.spekframework.spek2:spek-dsl-jvm:$spek_version") | ||
| testRuntimeOnly("org.spekframework.spek2:spek-runner-junit5:$spek_version") | ||
| testRuntimeOnly("org.jetbrains.kotlin:kotlin-reflect:$kotlin_version") | ||
| testImplementation("io.cucumber:cucumber-java:5.6.0") |
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.
with kotlin, better to use cucumber-java8 with lambda syntax
| } | ||
|
|
||
| class Server(private val port: Int) { | ||
| @Throws(Exception::class) |
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.
Throws annotation isn't needed, if you not use both kotlin and java in one project
| private val initializer = ServerChannelInitializer() | ||
| private lateinit var server: Server | ||
| private lateinit var expected: SetupFrame | ||
| private lateinit var actual: SetupFrame |
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.
actual not assigned?
2a1fa2f to
022e187
Compare
8bea67f to
fc06ba3
Compare
259fb1b to
6be3530
Compare
6be3530 to
08a599a
Compare
OlegDokuka
left a comment
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.
LGTM
No description provided.