Skip to content

Conversation

@aajtodd
Copy link
Contributor

@aajtodd aajtodd commented Nov 16, 2021

Issue #

sibling: smithy-lang/smithy-kotlin#536

Description of changes

Updates middleware and codegen to implement upstream interface changes

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

@github-actions
Copy link

A new generated diff is ready to view: __generated-main...__generated-refactor-middleware

@github-actions
Copy link

A new generated diff is ready to view: __generated-main...__generated-refactor-middleware

@github-actions
Copy link

A new generated diff is ready to view: __generated-main...__generated-refactor-middleware

@github-actions
Copy link

A new generated diff is ready to view: __generated-main...__generated-refactor-middleware

@aajtodd aajtodd marked this pull request as ready for review November 30, 2021 20:23
@aajtodd aajtodd requested a review from a team as a code owner November 30, 2021 20:23
@aajtodd aajtodd requested review from ianbotsf and kggilmer November 30, 2021 20:23
Copy link
Contributor

@ianbotsf ianbotsf left a comment

Choose a reason for hiding this comment

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

Fantastic!

Comment on lines +30 to 32
override fun install(op: SdkHttpOperation<*, *>) {
op.execution.mutate.register(this, Phase.Order.After)
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Question: Isn't Order.After the default? How is this different from the default implementation of install?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe it is yes. Probably just a miss, will double check. Good catch

Copy link
Contributor

@kggilmer kggilmer left a comment

Choose a reason for hiding this comment

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

again, love the reduction here.

writer.write("setIfMissing(#S, #S)", it.key, it.value)
writer.withBlock("op.install(", ")") {
withBlock("#T().apply {", "}", RuntimeTypes.Http.Middlware.MutateHeadersMiddleware) {
overrideHeaders.forEach {
Copy link
Contributor

Choose a reason for hiding this comment

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

nit:

I believe you can omit the writer. as your this is the writer via withBlock()

@sonarqubecloud
Copy link

sonarqubecloud bot commented Dec 7, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@github-actions
Copy link

github-actions bot commented Dec 7, 2021

A new generated diff is ready to view: __generated-main...__generated-refactor-middleware

@aajtodd aajtodd merged commit 5133dd0 into main Dec 7, 2021
@aajtodd aajtodd deleted the refactor-middleware branch December 7, 2021 14:46
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.

3 participants