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

Revamp errors in aws-smithy-checksums #1850

Merged
merged 2 commits into from
Nov 11, 2022
Merged

Conversation

jdisanti
Copy link
Collaborator

@jdisanti jdisanti commented Oct 13, 2022

Motivation and Context

This PR revamps errors in aws-smithy-checksums to adhere to RFC-0022: Error Context and Compatibility.

This PR is part of a series that will fully implement the RFC, tracked in #1926.

Changelog entries added in #1951.

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

@jdisanti jdisanti requested a review from a team as a code owner October 13, 2022 17:28
@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@jdisanti jdisanti force-pushed the jdisanti-errors-smithy-checksums branch from cab8db6 to 61c63d0 Compare October 17, 2022 23:24
@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@jdisanti jdisanti force-pushed the jdisanti-unhandled-error-newtype branch from 6e3e165 to 1b90d4c Compare October 21, 2022 18:07
@jdisanti jdisanti requested a review from a team as a code owner October 21, 2022 18:07
@jdisanti jdisanti force-pushed the jdisanti-errors-smithy-checksums branch from 61c63d0 to 5fe1e7d Compare October 21, 2022 18:07
@jdisanti jdisanti force-pushed the jdisanti-unhandled-error-newtype branch from 1b90d4c to ea102d4 Compare October 21, 2022 18:37
@jdisanti jdisanti force-pushed the jdisanti-errors-smithy-checksums branch from 5fe1e7d to 6338f4c Compare October 21, 2022 18:38
@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@jdisanti jdisanti force-pushed the jdisanti-unhandled-error-newtype branch from ea102d4 to b3a2b6c Compare October 28, 2022 22:43
@jdisanti jdisanti force-pushed the jdisanti-errors-smithy-checksums branch from 6338f4c to e053f34 Compare October 28, 2022 22:43
@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@jdisanti jdisanti marked this pull request as draft October 31, 2022 20:04
@jdisanti jdisanti force-pushed the jdisanti-unhandled-error-newtype branch from b3a2b6c to bd16b5d Compare November 2, 2022 20:41
@jdisanti jdisanti force-pushed the jdisanti-errors-smithy-checksums branch from e053f34 to 55c1535 Compare November 2, 2022 20:41
@github-actions
Copy link

github-actions bot commented Nov 2, 2022

A new generated diff is ready to view.

A new doc preview is ready to view.

import software.amazon.smithy.rust.codegen.core.rustlang.rustBlock
import software.amazon.smithy.rust.codegen.core.smithy.RuntimeType

internal fun unhandledError(): RuntimeType = RuntimeType.forInlineFun("Unhandled", RustModule.Error) {
Copy link
Contributor

@hlbarber hlbarber Nov 9, 2022

Choose a reason for hiding this comment

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

It might be more readable to do this in one rustTemplate rather than chopping it up?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good call. Not sure why I didn't think of that 😅

@@ -397,7 +376,7 @@ mod tests {
}

#[test]
#[should_panic = "called `Result::unwrap()` on an `Err` value: UnknownChecksumAlgorithm(\"some invalid checksum algorithm\")"]
#[should_panic = "called `Result::unwrap()` on an `Err` value: UnknownChecksumAlgorithmError { checksum_algorithm: \"some invalid checksum algorithm\" }"]
Copy link
Contributor

Choose a reason for hiding this comment

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

The reason we're should_panic here rather than asserting on the error type is because we don't want to derive PartialEq on the error?

Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be better to match on the error and then assert on the checksum_algorithm?

rust-runtime/aws-smithy-types/src/error.rs Outdated Show resolved Hide resolved
@jdisanti jdisanti force-pushed the jdisanti-unhandled-error-newtype branch from a15055e to 00fa06a Compare November 11, 2022 00:05
@jdisanti jdisanti force-pushed the jdisanti-errors-smithy-checksums branch from 55c1535 to 104d2aa Compare November 11, 2022 00:23
@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

Base automatically changed from jdisanti-unhandled-error-newtype to main November 11, 2022 21:57
@jdisanti jdisanti force-pushed the jdisanti-errors-smithy-checksums branch from 104d2aa to 6271b8c Compare November 11, 2022 22:01
@jdisanti jdisanti marked this pull request as ready for review November 11, 2022 22:01
@jdisanti jdisanti enabled auto-merge (squash) November 11, 2022 22:01
@github-actions
Copy link

A new generated diff is ready to view.

A new doc preview is ready to view.

@jdisanti jdisanti merged commit 6aef53a into main Nov 11, 2022
@jdisanti jdisanti deleted the jdisanti-errors-smithy-checksums branch November 11, 2022 22:29
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