-
-
Notifications
You must be signed in to change notification settings - Fork 269
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: fix invalid operation references in example snippets #620
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 and the instructions about a basic recommended setup 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.
bfb4e17
to
09ee46f
Compare
spec/asyncapi.md
Outdated
@@ -538,7 +539,9 @@ Field Pattern | Type | Description | |||
{ | |||
"user/signedup": { | |||
"subscribe": { | |||
"$ref": "#/components/messages/userSignedUp" | |||
"message": { | |||
"$ref": "#/components/messages/userSignUp" |
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 note, that I also changed userSignedUp
to userSignUp
here and in other locations. Before, both variations where used inconsistently in the spec.
I decided for userSignUp
because it was used more frequently and it's also the spelling used in the Message Object section.
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.
Yeah, thanks for the heads up. IMHO it should be userSignedUp
(past tense) but I don't think it matters a lot. Thanks for taking care anyway.
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.
+1 to rename it to use the simple past tense as it is common in events (Domain events, event-sourcing, cqrs, etc)
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 think originally, userSignUp
was used, because the example message given in the Message Object section is intended as a command. See for example this part: "summary": "Action to sign a user up.",
.
In this case, past tense wouldn't be appropriate imho. So if we want to change to past tense, we should adapt a couple of other wordings in the spec. 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.
We use both (command and event) interchangeably. I don't think we have to change anything here or in any other place. It's correct as it is, it was just a suggestion because most of the time we're speaking about events instead of commands. That's why we explicitly describe it when it's a command. For the rest of the occurrences, it should be an event. But really, not a blocker. It's ok as it is, at least for me.
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.
Alright, definitely not a hard opinion on the naming on my part. Even if it is not a blocker, since both of you have a preference towards the past tense names, I reverted the re-naming. So now, everything is as it was before naming wise.
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, only one minor about the name of the event. I agree @fmvilas event names are usually written in the simple past tense.
2baafd2
to
713ba1a
Compare
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
@all-contributors please add @balogal for doc and bug |
I've put up a pull request to add @balogal! 🎉 |
@all-contributors please add @balogal for doc and bug |
I've put up a pull request to add @balogal! 🎉 |
🎉 This PR is included in version 2.3.0-2022-01-release.1 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
🎉 This PR is included in version 2.3.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
title: Fix invalid operation references in example snippets
Related issue(s):
#619
Replaces invalid example snippets with valid ones. I decided to no longer include the message in the channel and parameter object section because I felt like it didn't add much to the understanding. But if you prefer, let me know and I add them as in the intro section.