feat: add option to generate named exports from Typescript generator #622
Conversation
gabormagyar
changed the title
There was a problem hiding this comment.
This is great!
Yes, we definitely need some tests, examples, and documentation to ensure this works as intended before it can be merged.
It seems you might have auto-formatting enabled in your development environment making unnecessary changes that make it harder to review, do you mind disabling it and re-commit the intended changes? 🙂
You can try to reach out on slack to see if you can find someone to help you finish it 🙂
|
Auto formatting removed |
|
@jonaslagoni Please advise if you think there's anything else that needs to be done. I personlally think this doesn't need a separate example. |
There was a problem hiding this comment.
Looks good tbh 👍
We do need to update the documentation though: https://github.com/asyncapi/modelina/blob/master/docs/languages/TypeScript.md#rendering-complete-models-to-a-specific-module-system
What's the reason you think an example is unnecessary? 🙂
Pull Request Test Coverage Report for Build 1999047854
💛 - Coveralls |
gabormagyar
requested review from
Samridhi-98,
magicmatatjahu and
asyncapi-bot-eve
as code owners
|
@jonaslagoni Added a note to the TypeScript docs. I don't think this is a big enough thing to add separate examples for. If we add examples for every combination of every possible option it will be a lot of examples. What do you think? |
|
Also interestingly the named exports don't really play nice with the generated description comments from the other PR (#621). It still generates valid code, it just looks ugly: the comments are between the export keyword and the node. I think it's fine for now, but in a future PR I will fix it so that the export keyword is generated through the preset system with priority over any comments. I think this will work but I'm open to suggestions, this is a rather tricky interaction. |
|
It might not be as fine as I thought. These comments are not compiled into the For reference, this is what these look like: export /**
* ...
*/
type name = ... |
|
Fixed, it's not pretty but it works and I do believe it's robust |
|
@jonaslagoni What are next steps for this PR? |
|
Kudos, SonarCloud Quality Gate passed!
|
|
/rtm |
|
🎉 This PR is included in version 0.53.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
|
🎉 This PR is included in version 1.0.0-next.23 🎉 The release is available on: Your semantic-release bot 📦🚀 |
This probably needs tests, I'm not sure when I will have time to add them. Feel free to take over if someone wants to.
It also needs
(minor)documentation changes about the added generator option.Correction:
TypeScriptRenderCompleteModelOptionsis wholly undocumented if I'm not mistaken, should probably be added at some point