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

feat: Rust codegen for Errors test model #573

Merged
merged 5 commits into from
Sep 11, 2024

Conversation

alex-chew
Copy link
Contributor

Issue #, if available: #534

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@alex-chew alex-chew marked this pull request as ready for review September 11, 2024 05:25
@alex-chew alex-chew requested a review from a team as a code owner September 11, 2024 05:25
@ajewellamz
Copy link
Contributor

ajewellamz commented Sep 11, 2024

These things are easier to review if you check in the *.rs files. We can always delete them in a following PR.

Along similar lines, maybe this was the PR to delete the *.rs files from Constructor, rather than updating them.

I see that the build() functions return aws_smithy_types::error::operation::BuildError, which are then wrapped in an Opaque, and so any actual message is forever lost to the customer. Is there any chance the we could define an additional Error variant for that instead?

That said, it's probably best to call this done for now, and make enhancements later.

@robin-aws
Copy link
Contributor

I see that the build() functions return aws_smithy_types::error::operation::BuildError, which are then wrapped in an Opaque, and so any actual message is forever lost to the customer. Is there any chance the we could define an additional Error variant for that instead?

Good callout that I think we can address in the scope of supporting Constraints (since that will actually add validation errors).

Even so, the message isn't actually lost, it's just buried and won't be printed out by default. Do we need to shoot for the display content of any unexpected error being printed when printing an Opaque? I'm not sure that's feasible, nor do the other languages try to achieve that AFAICT.

Copy link
Contributor

@robin-aws robin-aws left a comment

Choose a reason for hiding this comment

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

Awesome to have this done. Made non-blocking comments - they can be addressed in a future PR if necessary.


impl ::std::cmp::Eq for Error {}

impl ::std::fmt::Display for Error {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this not what #[derive(::std::fmt::Display)] would generate anyway?

Comment on lines +51 to +54
// Using Opaque since we don't have a validation-specific error yet.
// Operations' models don't declare their own validation error,
// and smithy-rs seems to not generate a ValidationError case unless there is.
// Vanilla smithy-rs uses SdkError::construction_failure, but we aren't using SdkError.
Copy link
Contributor

Choose a reason for hiding this comment

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

This comment should probably be a comment on the codegen code instead at this point.

IOUtils.evalTemplate(
"""
$rustErrorName:L {
message: ::std::string::String,
Copy link
Contributor

Choose a reason for hiding this comment

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

This is fine but reminded me I should cut #575

@@ -76,10 +76,13 @@ protected Stream<OperationShape> allOperationShapes() {
}

protected Stream<StructureShape> allErrorShapes() {
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI for full completeness this needs to also include all errors in any dependent services. But that's not possible to test since we can't build test models with dependencies yet. So this is just fine to hit the bar of supporting the Errors test model in this PR, but we will need to follow up with #576.

@robin-aws robin-aws merged commit 45efc82 into main-1.x Sep 11, 2024
80 checks passed
@robin-aws robin-aws mentioned this pull request Sep 11, 2024
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