-
-
Notifications
You must be signed in to change notification settings - Fork 346
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
orval/core - generating factory method for schema interfaces #1334
base: master
Are you sure you want to change the base?
orval/core - generating factory method for schema interfaces #1334
Conversation
78b0a4e
to
581b773
Compare
@@ -53,6 +53,7 @@ export const generateInterface = ({ | |||
} else { | |||
model += `export type ${name} = ${scalar.value};\n`; | |||
} | |||
model += `export function create${name}(): ${name} ${scalar.factoryMethodValue}\n`; |
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 will add this in the model file here. It's a bit weird no?
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... was thinking whether it should be in separate file or not. I've decided to leave it with model. Anyone who has imported a model, can either instantiate it through "new", or right away call a factory method - which increases the chances that random dev will learn about it and use it.
People are lazy and do not read the code - especially the one that's generated.
Hi, @mironbalcerzak |
64df5ba
to
76f3ec5
Compare
76f3ec5
to
9e5e347
Compare
9e5e347
to
32e6fab
Compare
0fb9a7d
to
27dbc05
Compare
27dbc05
to
e68e30d
Compare
e68e30d
to
5e5ece4
Compare
e0fde50
to
b77a14c
Compare
@soartec-lab - ive been using this branch in few projects of mine over last months - seems ok. |
It looks like the tests are failing? |
@melloware, yeah, i am aware and that will be fixed shortly - i meant on a broader scale - should i implement/change something in PR? |
i think once its fixed and passing we will re-review. |
@mironbalcerzak |
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.
@mironbalcerzak
I made some comments 👍
@@ -47,7 +47,7 @@ export const generateImports = ({ | |||
} } from \'./${upath.join(path, camel(name))}\';`; | |||
} | |||
|
|||
return `import ${!values ? 'type ' : ''}{ ${name}${ | |||
return `import { create${name}, ${name}${ |
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 create
exists in the http status, I want to change the function name to the new
prefix to avoid confusion.
return `import { create${name}, ${name}${ | |
return `import { new${name}, ${name}${ |
@@ -53,6 +53,7 @@ export const generateInterface = ({ | |||
} else { | |||
model += `export type ${name} = ${scalar.value};\n`; | |||
} | |||
model += `export function create${name}(): ${name} ${scalar.factoryMethodValue}\n`; |
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.
Factory methods are just noise for people who don't need them, so I'd like to be able to choose whether to generate them with a property.
For example, generateFactoryMethod: boolean
.
And by making it a property, people who want to use it will be aware that it will be generated. Therefore, you will be able to notice it even if you separate the file like .factory.ts
, right?
2cc176f
to
c9e3bb3
Compare
c20568d
to
91101bf
Compare
91101bf
to
45f4d4d
Compare
Status
WIP
Description
Fix #1331
This PR is providing simple implementation.
Opening for discussion.
Bugs? Possible - definitely "stackoverflow" in case of self referencing or cycle
Todos