Flatten shapes when fetching with Model.shape #269
Conversation
alextwoods
changed the title
mullermp
changed the title
|
Returns a flattened shape by default (resolving mixins and apply) when using Model.shape. Uses depth first recursion. Merging is based on DeepMergable |
| end | ||
| end | ||
|
|
||
| def apply_member_traits(id, shape) |
There was a problem hiding this comment.
shape.fetch('members', []) will work for shapes like structures and union. I think members for list (member) and map shapes (key, value) are defined differently so need to cover those cases, right?
I forget if this is already resolved from the get-go.
There was a problem hiding this comment.
I don't know if mixins work for those shapes or not but that's a good call out, I will investigate.
| Dir.glob('gems/smithy/spec/fixtures/**/model.smithy') do |model_path| | ||
| out_path = model_path.sub('.smithy', '.json') | ||
| sh("smithy ast --aut #{model_path} > #{out_path}") | ||
| config_files = smithy_build_files.map do |file| |
There was a problem hiding this comment.
Would there ever be multiple smithy build files? That seems maybe invalid?
There was a problem hiding this comment.
The AST command takes a config file and it says multiple ones are merged. The intent was to allow this to be at all folder hierarchies.
| if model['shapes'].key?(id) | ||
| Flattener.new(model).shape(id) | ||
| elsif PRELUDE_SHAPES.key?(id) | ||
| PRELUDE_SHAPES[id] |
There was a problem hiding this comment.
Is there any possible interaction between mixins and prelude shapes? I don't think there would be, but just want to check.
There was a problem hiding this comment.
I don't think so but I'll check.
There was a problem hiding this comment.
I tried this: Cannot apply smithy.api#mixin to an immutable prelude shape defined in smithy.api.
| deep_merge!(hash1, hash2, exclude_traits, context) | ||
| end | ||
|
|
||
| def deep_merge!(hash1, hash2, exclude_traits, context) |
There was a problem hiding this comment.
The difference between deep_merge and deep_merge! is confusing - maybe these could use comments?
There was a problem hiding this comment.
One returns a new hash and the other modifies the current hash, just like merge and merge!. I got inspiration from the rails implementations. I can add that here.
|
Changes overall look good but its my own PR so can't leave an approve. |
See @mullermp comment (hijacked PR)