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

Remove reserved types #1052

Merged
merged 16 commits into from
Jul 24, 2023
Merged

Remove reserved types #1052

merged 16 commits into from
Jul 24, 2023

Conversation

Baccata
Copy link
Contributor

@Baccata Baccata commented Jun 26, 2023

Remove the out-dated type-reservation list. The only things that are reserved are language keywords now.

Closes #1051

@Baccata
Copy link
Contributor Author

Baccata commented Jun 26, 2023

@miguel-vila, @kubukoz although my fix works and is actually much better in the grand scheme of things (no more reserved types), it has the annoying side effect of drastically impacting the UX for AWS clients, has AWS has the annoying tendency of redefining primitive types in all its specs :

https://github.com/aws/aws-sdk-js-v3/blob/main/codegen/sdk-codegen/aws-models/dynamodb.json#L10134-L10136

So I think we need to take action within this same unit of work to avoid this problem by means of a ProjectionTransformer that'd turn all simple shapes that are coming from aws-namespaces into references to the "standard" shapes (as long as the shapes we're transforming don't have any traits, and have the same names/shape-type as their targets). This ProjectionTransformer would be applied by default.

@miguel-vila, if you want to pick this, it'd be greatly appreciated

@daddykotex
Copy link
Contributor

@miguel-vila, @kubukoz although my fix works and is actually much better in the grand scheme of things (no more reserved types), it has the annoying side effect of drastically impacting the UX for AWS clients, has AWS has the annoying tendency of redefining primitive types in all its specs :

https://github.com/aws/aws-sdk-js-v3/blob/main/codegen/sdk-codegen/aws-models/dynamodb.json#L10134-L10136

So I think we need to take action within this same unit of work to avoid this problem by means of a ProjectionTransformer that'd turn all simple shapes that are coming from aws-namespaces into references to the "standard" shapes (as long as the shapes we're transforming don't have any traits, and have the same names/shape-type as their targets). This ProjectionTransformer would be applied by default.

@miguel-vila, if you want to pick this, it'd be greatly appreciated

Do you think they have something similar in-house? If they use those specs to generate their SDK, that'd mean, for example, that the Dynamo client works with dynamo.String rather than java.lang.String?

@Baccata
Copy link
Contributor Author

Baccata commented Jun 26, 2023

@daddykotex they don't have newtypes in house, so they always render the underlying type, which solves the problem (which obviously we don't want to do, because we have newtypes)

@kubukoz
Copy link
Member

kubukoz commented Jun 26, 2023

@Baccata so the transformation would:

  • replace occurrences of said types (no traits, unqualified name matches the underlying type) with the underlying type
  • remove said types from the model

and it would be included as a default smithy4sTransformations entry? or would you make it a default in another way?

@Baccata
Copy link
Contributor Author

Baccata commented Jun 27, 2023

replace occurrences of said types (no traits, unqualified name matches the underlying type) with the underlying type
remove said types from the model

absolutely

and it would be included as a default smithy4sTransformations entry? or would you make it a default in another way?

yes

@kubukoz
Copy link
Member

kubukoz commented Jul 7, 2023

I believe @denisrosca from my team will look into this next week. @Baccata given this is 0.17, I suppose you want to target this branch with the transformation PR, so that #1052 doesn't get released without that?

@Baccata
Copy link
Contributor Author

Baccata commented Jul 7, 2023

I suppose you want to target this branch with the transformation PR, so that #1052 doesn't get released without that?

Indeed I do

AWS in their smithy specs have a tendency to create newtypes for
the smithy standard shapes. As part of this change, introduce a
projection transformer that will run as a preprocessor before code
generation and will replace the newtypes (if they match some criteria)
with references to the standard smithy shapes.

For a newtype to be considered as being "replaceable" it has to match
the following criteria:
- doesn't have any traits (other than @default)
- is a simple shape
- the name matches the smithy shape (one exception is Date, as it
will be replaced with Timestamp even though it doesn't actually match).
- comes from an AWS namespace (i.e. starts with com.amazonaws)
@denisrosca
Copy link
Contributor

@kubukoz @Baccata

I opened #1110 to remove AWS newtypes during code generation.

@Baccata Baccata merged commit 56f64c3 into series/0.17 Jul 24, 2023
12 checks passed
@Baccata Baccata deleted the remove-reserved-types branch July 24, 2023 12:10
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.

5 participants