Skip to content
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: update examples to be written by human not converter #977

Merged

Conversation

derberg
Copy link
Member

@derberg derberg commented Oct 10, 2023

  • adjusted all the channel names, messages names and operations names that were generated by converter to be more human friendly. My main concert was that having a generated channel name that is the same as address would bring confusion and suggest bad practice
  • added if few places missing quotations - related conversion to v3 do not preserve strign quotes converter-js#197
  • websocket kraken had few issues that needed a fix as they were kinda written still from a perspective of the user
  • the other changes are commented inline

What I did not do
So social media example could be modified further imho. It was limited by v2 and could not show sharing of channels and operations. Now I think it should be refactored to do so too. Wdyt?

@@ -148,7 +148,15 @@ components:
description: Stands for "Gravatar version" and is used for cache busting.
bindings:
http:
$ref: '#/components/messageBindings/streamingHeaders'
headers:
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$ref no supported in http header

@@ -159,10 +167,12 @@ components:
- "\r\n"
bindings:
http:
$ref: '#/components/messageBindings/streamingHeaders'
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ref not supported in header + it must be a json schema

@@ -28,15 +28,8 @@ operations:
- 'subscribe:auth_revocations'
bindings:
http:
type: request
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not supported in operation anymore, same headers

@@ -48,6 +41,10 @@ components:
X-SIGNATURE:
description: ECC message signature
type: string
Content-Type:
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

header can be normally a part of the message

requestSum.message:
bindings:
amqp:
replyTo:
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not supported by binding

sum.message:
bindings:
amqp:
replyTo:
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not supported by binding

$ref: '#/components/messages/book'
description: >-
Every time a resource of type `http://schema.org/Book` is created or
modified, a JSON-LD representation of the new version of this resource
must be pushed in this Mercure topic.
parameters:
id: {}
id:
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this one was converted correct, as type was integer and no more info, even description provided, so result is basically an empty id parameter. But yeah, for best practice better share that description is a minimum

@sonarcloud
Copy link

sonarcloud bot commented Oct 10, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

commentId: {}
update/comment/likes:
commentId:
$ref: '../common/parameters.yaml#/commentId'
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since it is no longer schema it is better to promote also a separate file per parameters

@derberg derberg marked this pull request as ready for review October 10, 2023 13:38
Copy link
Collaborator

@dalelane dalelane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@fmvilas
Copy link
Member

fmvilas commented Oct 30, 2023

/rtm

@asyncapi-bot asyncapi-bot merged commit cc109fe into asyncapi:next-major-spec Oct 30, 2023
43 of 45 checks passed
@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 3.0.0-next-major-spec.15 🎉

The release is available on GitHub release

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants