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

Fix TSApplicationGenerator to use Result<> instead of #{Result} #4031

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

drganjoo
Copy link
Contributor

@drganjoo drganjoo commented Feb 20, 2025

The earlier PR, which ensured that a Smithy shape named Result does not conflict with the Rust type Result, had a bug in TSApplicationGenerator. The rustTemplateBlock was using #{Result} but preludeScope was not passed in as a parameter.

This bug can be addressed in two ways:

  1. Pass *RuntimeType.preludeScope to the rustBlockTemplate call
  2. Remove #{Result} from rustBlockTemplate and continue using Result<>

This PR implements the second approach because:

  1. TSApplicationGenerator is slated for deprecation
  2. The CI step CI / PR Bot / Generate diff and upload to S3 (pull_request) currently fails when comparing codegen between HEAD and this version. Removing #{Result} is the most reliable way to avoid parameter-passing issues with rustBlockTemplate

@drganjoo drganjoo force-pushed the fahadzub/fix-ts-result branch from b105a8d to c35306f Compare February 20, 2025 19:58
@drganjoo drganjoo force-pushed the fahadzub/fix-ts-result branch from 80187aa to dd500b6 Compare February 20, 2025 21:16
@drganjoo drganjoo marked this pull request as ready for review February 21, 2025 09:21
@drganjoo drganjoo requested a review from a team as a code owner February 21, 2025 09:21
@drganjoo drganjoo changed the title Add preludeScope to TSApplicationGenerator Fix TSApplicationGenerator to use Result<> instead of #{Result} Feb 21, 2025
@drganjoo drganjoo enabled auto-merge February 21, 2025 09:24
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