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

Support include-prelude in smithy ast #2500

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

mullermp
Copy link
Contributor

Background

  • What do these changes do?

It allows you to get prelude shapes when getting the AST.

  • Why are they important?

Implementations that do not use Smithy in Java will need a way to get prelude shapes in the JSON model. Otherwise, special casing is necessary to handle prelude shapes.

Testing

clean, build, then smithy-cli:runtime.

smithy-cli/build/image/smithy-cli-darwin-aarch64/bin/smithy ast --include-prelude will include prelude shapes.


By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@mullermp mullermp requested a review from a team as a code owner January 14, 2025 23:56
@mullermp mullermp requested a review from joewyz January 14, 2025 23:56
@sugmanue
Copy link
Contributor

It allows you to get prelude shapes when getting the AST.

Can you please elaborate on why is this needed? What use case are you trying to solve that needs the prelude shapes in the serialized model?

@mullermp
Copy link
Contributor Author

Regardless of other implementations, I think being able to see the prelude shapes in the output of AST is useful, especially if those implementations need to see those definitions, or if you want a complete closure of the model to generate against.

The closure I think is important so that implementations don't need to know about preludes at all. When I'm given a model, I expect that model to be complete. I otherwise need to special case every prelude ID. For example, if a structure targets a string, I can't visit that string, I need to inspect all keys of the structure and ignore prelude keys. If the string prelude is in the model, then I can iterate all members agnostically and check their type field, and visit shapes that way.

Copy link
Contributor

@yasmewad yasmewad left a comment

Choose a reason for hiding this comment

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

Can you add unit tests for the flag?

@sugmanue
Copy link
Contributor

sugmanue commented Jan 23, 2025

Regardless of other implementations, I think being able to see the prelude shapes in the output of AST is useful, especially if those implementations need to see those definitions, or if you want a complete closure of the model to generate against.

I don't feel like this is useful enough to justify this change, in particular because, as discussed offline, the prelude shapes need to be known ahead of time anyway. It's also possible to configure the build to include them without any need for this change, see here.

@mullermp
Copy link
Contributor Author

How do you use a smithy-build.json with the AST command?

@sugmanue
Copy link
Contributor

How do you use a smithy-build.json with the AST command?

In general you can use -c as in

smithy ast -c smithy-build.json

But it doesn't seem to work as expected, not sure why.

@mullermp
Copy link
Contributor Author

Yes, I tried that originally. I also don't know why a build config file shouldn't influence the AST command, so I ended up opening this PR. If smithy build can send me a complete model with prelude if configured to do so, AST should as well.

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