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

chore: support sync rust #760

Merged
merged 18 commits into from
Jan 22, 2025
Merged

chore: support sync rust #760

merged 18 commits into from
Jan 22, 2025

Conversation

ajewellamz
Copy link
Contributor

Issue #, if available:

Description of changes:

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

@ajewellamz ajewellamz requested a review from a team as a code owner January 13, 2025 20:42
robin-aws
robin-aws previously approved these changes Jan 22, 2025
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.

LGTM assuming we sort out the CI failures

@@ -853,7 +853,7 @@ private RustFile operationOuterModule(
variables.put(
"operationSendBody",
evalTemplate(
"$snakeCaseResourceName:L.inner.borrow_mut().$snakeCaseOperationName:L(input)",
"$snakeCaseResourceName:L.inner.lock().unwrap().$snakeCaseOperationName:L(input)",
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume this works in non --sync-rust mode as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No. smithy-dafny only works in sync mode.
You told me to do that.

Copy link
Contributor

Choose a reason for hiding this comment

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

Yup I'm fine with that, I was just curious.

Comment on lines +25 to +28
let result = resource
.inner
.lock()
.unwrap()
Copy link
Contributor

Choose a reason for hiding this comment

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

Not blocking on this PR, but I think we should really have resource operations defined on the resource ref structs as well, so customers doesn't have to mess with internal things. borrow_mut() wasn't great before in terms of encapsulation and now it's a bit worse.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that would be very helpful. Especially if we could then make inner private.
Should the be an issue or a sim or something?

Copy link
Contributor

Choose a reason for hiding this comment

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

@robin-aws robin-aws merged commit ce73393 into main-1.x Jan 22, 2025
86 checks passed
@ajewellamz ajewellamz deleted the ajewell/sync-rust branch January 22, 2025 23: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.

2 participants