Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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: add documents and postinstallation scripts for ag deprecation #1251
base: master
Are you sure you want to change the base?
docs: add documents and postinstallation scripts for ag deprecation #1251
Changes from 4 commits
3b2002a
d5402c8
ef5e6b4
709b694
3a8f015
e063238
6ab60db
9e7f010
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
remember that users do not understand waht
cli.js
is, only maintainers. This notice is for users and they only know that cli isag
orasyncapi-generator
also please use one of GH supported markdown alerts -> https://docs.github.com/en/get-started/writing-on-github/getting-started-with-writing-and-formatting-on-github/basic-writing-and-formatting-syntax#alerts. Probably
warning
is what we need.and last but no least:
/apps/generator/docs/deprecate-cli.sj-ag.md
transform to a link pointing tohttps://asyncapi.com/docs/tools/generator/cli-deprecation
- yes, link is 404 but will not be after we merge this PR. Of course you need to renamedeprecate-cli.sj-ag.md
file tocli-deprecation.md
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.
For the GH supported markdown alerts, do you mean this?
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.
as in my other comment, it is not a source file that we deprecate but the AsyncAPI Generator CLI that has different technical name. See how others still promote it in template readme files -> https://github.com/asyncapi/java-spring-cloud-stream-template?tab=readme-ov-file#how-to-use-this-template as
ag
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.
great work of preparing these equivalents!!!
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 guess here and in others, word
equivalent
should start with lowercase?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.
please add note that there are different methods that do not require setup of node environment -> https://www.asyncapi.com/docs/tools/cli/installation
the best probably would be to use https://github.com/asyncapi/website/blob/master/assets/docs/fragments/cli-installation.md fragment - this is how to use it -> https://github.com/asyncapi/website/blob/master/markdown/docs/tutorials/generate-code.md?plain=1#L32
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.
if above
ag
example as-o
or-p
wouldn't it be better to also show the same here? instead of--output
and--param
.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'm not sure we really need this one, even if optional
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 know we talked about postinstallation as best option, but to be honest, I'm still in doubts. This notice will show up every-time someone installs generator, even if it is a dependency to AsyncAPI CLI - so during AsyncAPI CLI installation - at least I think so - something you would have to verify
this means we will spam all generator users, even the ones not using
ag
🤔 maybe in the end it is better to have a warning invoked only as part of cli.js code? even if it runs every time someone invokes it? 🤔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.
hmmmm my concern with having a warning only for invoking cli.js is that it would not be obvious? Since a lot of users probably already have cli.js as part of their code's workflow and don't really look at it anymore? But I can see this is also a problem for having a message during installation.
If spamming users is a problem, I can shorten the message a bit so it would be as annoying. What do you think?
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.
better just link to the deprecation guide
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.
not
node Cli.js