-
-
Notifications
You must be signed in to change notification settings - Fork 232
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
docs: create the model generation guide #1096
docs: create the model generation guide #1096
Conversation
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 need to adjust narrative in the main document as the scope of #1042 was not a tutorial, but more of a guide that tells you what is needed and shows an example.
- we do not need to mention https://github.com/derberg/python-mqtt-client-template. You can just add a note that the example you provide can be integrated in a template that one can create by following https://www.asyncapi.com/docs/tools/generator/generator-template
- I think it is enough that first step of the guide just says that
@asyncapi/modelina
needs to be added to template dependencies asnpm install --save @asyncapi/modelina
because then it is automatically added topackage.json
Model generation using the AsyncAPI CLI
section can be removed as it is not adding anything new, template with or without modelina - generation is always the same. You can just link to https://www.asyncapi.com/docs/tools/generator/usage#generator-cli (btw we have error there as section should be called AsyncAPI CLI, and we should also have a link to https://www.asyncapi.com/docs/tools/cli there)- please remember about proper linking inside asyncapi.com with relative links and also consistent usage of upper/lower cases with names of tools
also, in your PR can you add between line 37 and 38 in https://github.com/asyncapi/generator/blob/master/.github/workflows/update-docs-in-website.yml#L37-L38 a line rm -r ./pages/docs/tools/generator
to always have a clean and empty state of generator docs folder in website? Problem is that we now do not remove, and just add, so when we rename files like this https://github.com/asyncapi/generator/blob/master/docs/generator-template.md then we now get a duplicated tutorial in website
editorial from Lukasz Co-authored-by: Lukasz Gornicki <lpgornicki@gmail.com>
Hey @derberg, I have addressed all the feedback you left on this PR |
@derberg this is ready for a second review |
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.
left one big comment
please also update the title and description of PR - we do not merge tutorial with this PR
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.
Review from Lukasz
Co-authored-by: Lukasz Gornicki <lpgornicki@gmail.com>
@Florence-Njeri please have a look at this comment with big modification - there was suppose to be much more content, not just finish with adding dependency 🤔 also do not forget about 👇🏼
|
sorry @Florence-Njeri my comment was weirdly interpreted by GitHub commit suggestion pasting the same comment below: title: "Adding models generation in template"
|
Hello, @derberg! 👋🏼
|
Quality Gate passedKudos, no new issues were introduced! 0 New issues |
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.
@jonaslagoni @kennethaasan @magicmatatjahu since this is Modelina related, anything from your side? is it a good way to promote Modelina as component that enables models generation in app template?
maybe long term would be better to have a dedicated React component that makes it even easier to embed model generation - but for now I think it should be enough, especially that there is nothing else at the moment
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.
LGTM 👍
/rtm |
🎉 This PR is included in version 1.17.1 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Description
Related issue(s)
Resolves #1042