Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
[Resolve #1318] Implement dump template and write output files #1325
[Resolve #1318] Implement dump template and write output files #1325
Changes from 29 commits
4d1e5cc
c9ebb5a
97d680f
8ad7b63
3005b14
acb399b
a3bf4b7
c1ca8ed
bfa4675
fc2be81
8905933
7c1d2a1
901c5cf
8d1bbdd
f9533bd
7919be0
efb2090
2ec7239
7899dad
c7cf881
b95a914
1b7a8d2
a9a9076
d21f7dd
7e8629b
323e3e8
e755dbf
22a0516
5bbfb6f
119162d
c1b3068
94dcaf1
3e9682e
e5321c0
64004ff
d08f091
8a771e6
08862a8
f51e267
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
This is missing the
--to-file
option. is this intentional?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.
not intentional, fixed.
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.
I find it odd that we still echo if a file_path is specified.
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.
Ok how about now? (I'm not sure whether people want the text to go to both the screen and file if file_path is specified).
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.
You seem to have a very specific understanding of what this "template" ought to be: A dict with a single key, which is the stack name, and a single value, which is the template dict that needs to be written.
It's not super obvious from reading this code that this is supposed to work this way. Again, as an inner, nested function, it was fine. But now that you've moved it out to be at the level of "helper framework", it's not a simple or well-described interface. In fact, it's a misnomer to call it the "template", when in reality it's something more and different than just the "template". And I don't see why it should be the interface at all. Instead, I think that this function should take a
stack_name
and atemplate_dict
without this single-key dictionary structure business going on. It'd be easier to follow and reuse that way.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.
You'll have to elaborate ....
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.
Your
template
variable here isn't actually a template. In other words, your variable naming lies. For a helper function that's meant to stand on its own, the interface must be clear, simple, and functional. But you're making a lot of assumptions here about whattemplate
is.It should take a
stack_name
and atemplate_dict
parameter instead of a dict that happens to have a key that is the stack name and a value that is the actual template dict. Your code would be simpler, more readable, and easier to interact with if you did that.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.
I'm not convinced that a variable named
template_dict
of typedict
is clearer thantemplate: dict
and to my mind the whole comes out more rather than less messy afterwards. Perhaps we are better off without this helper at all and I revert to how it was before?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.
For example on the caller side:
We don't want duplication of
list(template.keys())[0]
all over the place for the sake of helping the reader of that helper function since most people are going to be reading the CLI functions; not the helpers.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.
My point is that you're calling the dict a "template", but it's actually a dict with a single key indicating the stack name and a its value is the actual template. In other words, your variable name is wrong and the interface of the function is more challenging than it ought to be.
Rather than assuming this arbitrary structure that is a mouthful to describe, I'm just saying take two parameters instead of this complex dict structure that is isn't well-typed.
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.
You ought to deprecate
generate
above as well.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.
done