-
Notifications
You must be signed in to change notification settings - Fork 52
Refactor codegen.ts #110
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
Refactor codegen.ts #110
Conversation
Splitting out the not exported code from codegen.ts to make it easier to read.
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 great, thanks for contributing!
I left a few minor comments, should be very quick to address :)
Making it clear that the abstraction is designed with purpose. mtennoe#106
src/generators/swagger2.ts
Outdated
} | ||
|
||
export const Swagger2Gen: CodeGenerator = { | ||
getViewData: function(opts: CodeGenOptions): ViewData { |
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.
function [](start = 15, length = 8)
Personal preference: Should we convert these to arrow functions instead? that would look slightly cleaner to me here, and we could remove some redudnant typing. I.e.
export const Swagger2Gen: CodeGenerator = {
getViewData: opts => {
verifyThatWeAreGeneratingForSwagger2(opts);
return getViewForSwagger2(opts);
},
getCode: opts => {
const data = this.getViewData(opts);
return transformToCodeWithMustache(data, opts.template, opts.mustache);
}
};
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.
Agree, that makes it cleaner and the scope easier to follow.
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.
@mtennoe I have updated it now.
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.
@erictuvesson - Great!
There seems to be some minor build issues though that needs to be addressed
@erictuvesson Merged and published as 3.0.5, thanks! |
Splitting out the not exported code from codegen.ts to make it easier to read.
There are a few exported methods in
CodeGen
that I don't really see a good use, I didn't want to remove anything for compatibility.Microsoft Reviewers: Open in CodeFlow