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
feat: write samples to file (pt3) #980
feat: write samples to file (pt3) #980
Changes from 16 commits
4665842
4906d27
cf7b00a
ce46d8d
49f2c08
665a1d1
9309fd1
a56359b
8aaf96e
9df6556
6816855
1574de6
38301a2
98ef29d
352d7d6
d06de38
ace4541
11fa1ae
fd1ec87
5abb7a3
c1633a0
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 method is basically adding region tags and headers to all samples in all classes, which should probably be renamed as well. But before you do that, I have a basic question regarding why we want to do it here after samples are already mostly composed? The package info is already in the context which is passes into
generateServiceClasses
, so potentially we should be able to generate a full Sample in previous steps right?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.
Before this step, we can generate an inline sample but not an "executable" one because inline format doesn't need a proper region tag or license. Looking at composeServiceClasses called in Generator before Writer I could parse shortname/version out before or start of composeServiceClasses() and keep passing these details around when creating Samples or I can add these details before writing the files as I'm doing. I already am iterating over the samples to add the license header here and these details aren't needed before this step.
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.
Yeah, it's unfortunate these info are not easily accessible and you have to pass them around. I have some refactoring ideas but should definitely not be the scope of this PR. For now, can you please rename this method to make more readable/precise?
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.
Makes sense and I agree it's not ideal, assuming the behaviors won't change I can test this out overall and refactor in parallel.
done, I renamed to make more readable