-
-
Notifications
You must be signed in to change notification settings - Fork 108
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
fix : Modify code generation script to include DCS Concerto model types #952
base: main
Are you sure you want to change the base?
fix : Modify code generation script to include DCS Concerto model types #952
Conversation
Signed-off-by: Ayush1404 <[email protected]>
360fa67
to
d5fcd46
Compare
@sanketshevkar hey , i had to make little changes to make it work , but i have pushed the commits after running codegen script |
@Ayush1404 please run unit tests before you push your changes. Unit tests are failing. |
Signed-off-by: Ayush1404 <[email protected]>
d64187a
to
8034908
Compare
@sanketshevkar hey , sorry the build was failing cause i changed the exports , i have fixed them in the recent commit . What do i do next ? |
@@ -813,3 +813,4 @@ class DecoratorManager { | |||
} | |||
|
|||
module.exports = DecoratorManager; |
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.
module.exports = DecoratorManager; | |
module.exports = { DCS_MODEL, DecoratorManager }; |
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.
Can you please export this using the index.js?
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.
But doing so will give an error as tests expect DecoratorManager to be exported as default , we can re-export it in the index.js but we have to keep the default export for tests to succeed .
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.
Hmmm, I'm wondering we could just fix the tests with imports, but could this could break for someone whose not importing through index.js
. Let me discuss this during today's WG call.
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 just discussed, we decided to move DCS_MODEL
to concerto-metamodel
package. I'll raise a PR and cut a release and update it here to be used.
Can you please wait until I move things there?
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.
@sanketshevkar yeah sure , no problem . I would like to contribute , could you direct me to something where i could ?
Co-authored-by: Sanket Shevkar <[email protected]> Signed-off-by: Ayush Kadam <[email protected]>
Closes #943
Modifed code generation script to include DCS Concerto model types
Changes
Related Issues