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

Add Scoped Plugin #2759

Merged
merged 15 commits into from
Jun 14, 2023
Merged

Add Scoped Plugin #2759

merged 15 commits into from
Jun 14, 2023

Conversation

hlbarber
Copy link
Contributor

@hlbarber hlbarber commented Jun 9, 2023

Motivation and Context

The FilterByOperationName allows the customer to filter application of a plugin. However this is a runtime filter. A faster and type safe alternative would be a nice option.

Description

Add Scoped Plugin and scope macro.

@hlbarber hlbarber added the server Rust server SDK label Jun 9, 2023
@github-actions
Copy link

github-actions bot commented Jun 9, 2023

A new generated diff is ready to view.

  • AWS SDK (ignoring whitespace)
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • No codegen difference in the Server Test Python
  • No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

@github-actions
Copy link

A new generated diff is ready to view.

  • No codegen difference in the AWS SDK
  • No codegen difference in the Client Test
  • No codegen difference in the Server Test
  • Server Test Python (ignoring whitespace)
  • No codegen difference in the Server Test Typescript

A new doc preview is ready to view.

Base automatically changed from harryb/simplify-plugin to main June 12, 2023 17:10
@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

Copy link
Contributor

@david-perez david-perez left a comment

Choose a reason for hiding this comment

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

I like the idea, implementation is clever.

I'd add some docs with examples highlighting when to choose FilterByOperationName or this, it's going to be the first thing someone thinks about when learning about both.

Changelog entry is missing.

rust-runtime/aws-smithy-http-server/src/plugin/scoped.rs Outdated Show resolved Hide resolved
rust-runtime/aws-smithy-http-server/src/plugin/scoped.rs Outdated Show resolved Hide resolved
@@ -111,6 +111,8 @@ mod filter;
mod identity;
mod layer;
mod pipeline;
#[doc(hidden)]
Copy link
Contributor

Choose a reason for hiding this comment

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

Why hidden?

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'd like Scoped and the scope macros to be the primary API and the #[doc(hidden)] items to be implementation details.

rust-runtime/aws-smithy-http-server/src/plugin/scoped.rs Outdated Show resolved Hide resolved
@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@hlbarber hlbarber force-pushed the harryb/scoped-plugin branch 4 times, most recently from fc7c8ea to f96e6ab Compare June 14, 2023 08:48
@hlbarber hlbarber marked this pull request as ready for review June 14, 2023 08:49
@hlbarber hlbarber requested review from a team as code owners June 14, 2023 08:49

/** Calculate all `operationShape`s contained within the `ServiceShape`. */
private val index = TopDownIndex.of(codegenContext.model)
private val operations = index.getContainedOperations(codegenContext.serviceShape).toSortedSet(compareBy { it.id })
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
private val operations = index.getContainedOperations(codegenContext.serviceShape).toSortedSet(compareBy { it.id })
private val operations = index.getContainedOperations(codegenContext.serviceShape).toSortedSet(compareBy { it.id })

"SmithyHttpServer" to ServerCargoDependency.smithyHttpServer(runtimeConfig).toType(),
)

/** Calculate all `operationShape`s contained within the `ServiceShape`. */
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/** Calculate all `operationShape`s contained within the `ServiceShape`. */

}

fun render(writer: RustWriter) {
macro()(writer)
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 feel like you can just inline.

rust-runtime/aws-smithy-http-server/src/plugin/scoped.rs Outdated Show resolved Hide resolved
@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@hlbarber hlbarber enabled auto-merge June 14, 2023 13:23
@hlbarber hlbarber added this pull request to the merge queue Jun 14, 2023
@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@hlbarber hlbarber removed this pull request from the merge queue due to a manual request Jun 14, 2023
@hlbarber hlbarber enabled auto-merge June 14, 2023 13:54
@hlbarber hlbarber added this pull request to the merge queue Jun 14, 2023
@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

Merged via the queue into main with commit 988eb61 Jun 14, 2023
@hlbarber hlbarber deleted the harryb/scoped-plugin branch June 14, 2023 14:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
server Rust server SDK
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants