-
Notifications
You must be signed in to change notification settings - Fork 6
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
Flatten shapes when fetching with Model.shape #269
base: decaf
Are you sure you want to change the base?
Conversation
gems/smithy/lib/smithy/model.rb
Outdated
def preprocess(model) | ||
# TODO: Deep dup of model | ||
model['shapes'].each do |id, shape| | ||
flatten_shape(id, model, shape) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since we are flattening the shape here (bringing members and its traits from the mixin shape and applying to this specific shape), do we keep around the mixin shape?
I'm guessing once all the shapes that uses mixins are resolved - the shapes that are marked with mixins are not quite useful since they already fulfilled their purpose. I'm not sure if we opt to remove them or skip over them during codegeneration (check if mixin
trait is applied, etc)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh yeah, thats actually a good point. The flattten_shape
ends up removing the mixin
property of shapes that has flattened, but the shapes themselves remain on the model. Probably makes sense to remove those mixins as well. In theory they shouldn't get picked up when we're using the service shape closure, but they could be picked up if we don't have a service (depending on how we end up implementing that down the road).
Description of changes:
Demo of option 1 - implementing a preprocess for the model that is applied during plan initialization and which flattens mixins and applies.
TODO: This does NOT correctly handle trait and member order. The prioritization logic of merge is correct, but the resulting order of items is not. We may need to implement our own merge method.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.