-
-
Notifications
You must be signed in to change notification settings - Fork 34
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
feat: toc and operation reply #523
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.
Welcome to AsyncAPI. Thanks a lot for creating your first pull request. Please check out our contributors guide useful for opening a pull request.
Keep in mind there are also other channels you can use to interact with AsyncAPI community. For more details check out this issue.
/ptal |
@fmvilas @derberg @magicmatatjahu Please take a look at this PR. Thanks! 👋 |
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.
@honnel @CodeGlitcher please join review at least as end users, not for code.
Just grab this PR and run generation locally with your files and share feedback
instead of usual @asyncapi/markdown-template
you just need to put https://github.com/korifey91/async-api-markdown-template
Generated TOC looks good to me, nice work! 🎉 |
I rendered a preview of our asycn api documents and it looks good to us. |
helpers/common.js
Outdated
REQUEST: 'request', | ||
SEND: 'publish', | ||
REPLY: 'reply', | ||
RECEIVE: 'subscribe', |
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.
v2 did not support request/reply pattern, so we really need to stick to publish/subscribe always, no request/reply
what I mean is that in case of v2 it is either publish
or subscribe
and I believe operation.reply()
is anyway always undefined in v2
not sure if you are familiar in v2
so in v2 spec, we had publish/subscribe from perspective of the user of the API. So if AsyncAPI document for service A had publish
operation - then it meant that end user can send a message, and service A in fact subscribes to a message. Same around, if service A had subscribe
operation, it meant that user can subscribe
so the service A was in fact publishing
. Yeah, confusing, thus we changed it
in v3 we say what API does, so send
means API sends and receive
means API receives - as simple as that.
REQUEST: 'request'
should be publish
REPLY: 'request'
should be subscribe
any anyway, they will never be use as in v2, reply is always undefined
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.
Thank you for detailed comment! I appreciate it.
I'll fix types.
@korifey91 lemme know whenever you're ready for next review |
@derberg I'm ready. Take a look, please |
@korifey91 I changed pr prefix to please have a look into failing tests |
We need someone we can trust to check the tests and ensure they are accurate. There are some issues with the snapshots that seem incorrect to me, as they do not contain the necessary information. For example,
As we can see here it expects one line of a text(which seems incorrect to me). There are fails with the same problems. Except one, with a new |
@derberg If you trust me, I can override snapshots and fix the test with |
yeah, they are human readable, so just update and push to PR and I will see what changed, no worries |
…ment` from `@asyncapi/parser`
…hould render bindings for operation`
Quality Gate passedIssues Measures |
@derberg done, please take a look |
LGTM! Thanks @korifey91 a 💯 if only all PR were on such good quality level! @honnel @CodeGlitcher would it be a lot if I ask you again to test this PR 🙏🏼 there was some refactoring done after you had a look. Snapshots look good to me, but better double check 😄 |
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.
ok, looks like people do not have more feedback to share
thanks @korifey91 again
/rtm |
@allcontributors please add @korifey91 for code,test |
I've put up a pull request to add @korifey91! 🎉 |
dependabot is such an ass..... pushed updates while release pipeline was in progress, and blocked release :( |
@korifey91 maybe you would be willing to open another quick fix PR for #365 and then we would trigger release again |
I'll take a look and try to find some time for fix |
🎉 This PR is included in version 1.6.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Description
Notes:
If you are interested in this pull request, please guide me on how we can merge it.
Related issue(s)
Resolves #454, #452