-
Notifications
You must be signed in to change notification settings - Fork 187
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
Partial HTTP protocol implementation #1
Conversation
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. Minor items noted.
} | ||
} else { | ||
this.block(outerField) | ||
} | ||
} | ||
|
||
fun ListForEach(target: Shape, outerField: String, block: CodeWriter.(field: String, target: ShapeId) -> Unit) { |
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.
nit: camelcase
return if (nullableIndex.isNullable(shape) && !shape.hasTrait(HttpLabelTrait::class.java)) { | ||
private fun handleOptionality(symbol: Symbol, member: MemberShape, container: Shape): Symbol { | ||
val httpLabeledInput = container.hasTrait(SyntheticInput::class.java) && member.hasTrait(HttpLabelTrait::class.java) | ||
return if (nullableIndex.isNullable(member) && !httpLabeledInput) { | ||
val builder = Symbol.builder() | ||
val rustType = RustType.Option(symbol.rustType()) |
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.
One syntatic niceity is with(builder) {}
allows you to remove redundant type names when accessing properties/functions on the same instance line over line. (FYI)
@@ -91,6 +92,7 @@ class SymbolVisitor( | |||
) : SymbolProvider, | |||
ShapeVisitor<Symbol> { | |||
private val nullableIndex = NullableIndex(model) | |||
private val bottomUpIndex = BottomUpIndex(model) |
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.
Seems unneeded
|
||
fun Symbol.Builder.rustType(rustType: RustType): Symbol.Builder { | ||
return this.putProperty(RUST_TYPE_KEY, rustType) | ||
} | ||
|
||
fun Symbol.rename(newName: String): Symbol { | ||
assert(this.rustType() is RustType.Opaque) |
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.
Consider check
for this
import software.amazon.smithy.rust.codegen.smithy.RuntimeConfig | ||
import software.amazon.smithy.rust.codegen.smithy.RuntimeType | ||
|
||
data class ProtocolConfig( |
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.
Nice
val memberType = model.expectShape(memberShape.target) | ||
val memberSymbol = symbolProvider.toSymbol(memberShape) | ||
val memberName = symbolProvider.toMemberName(memberShape) | ||
OptionForEach(memberSymbol, "&self.$memberName") { field -> |
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.
nit: case of these functions seems off
@@ -22,7 +24,7 @@ fun String.runCommand(workdir: Path? = null): String? { | |||
proc.waitFor(60, TimeUnit.MINUTES) | |||
if (proc.exitValue() != 0) { | |||
val output = proc.errorStream.bufferedReader().readText() | |||
throw AssertionError("Command Failed\n$output") | |||
throw CommandFailed("Command Failed\n$output") |
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.
you could also just do error("Command Failed...")
here.
Description of changes:
This diff adds extremely limited support for
AwsJson1.0
andRestJson1
. Along the way it fixes lots of bugs & does assorted refactorings to clean up abstractions. It's not really a 10k line diff: 9,150 of the lines are from adding Dynamo and EBS as integration tests.Currently, the protocol implementations are inside of Smithy—we may decide that that needs to be refactored later, but TBD.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.