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 readme.md with overview and custom validation sections #243

Merged
merged 23 commits into from
Jul 29, 2022

Conversation

Dindihub
Copy link
Contributor

@Dindihub Dindihub commented Jul 19, 2022

Copy link

@github-actions github-actions bot left a 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.

@Dindihub Dindihub changed the title docs:Update README.md with overview and custom validation sections docs: update readme.md with overview and custom validation sections Jul 19, 2022
Copy link
Member

@derberg derberg left a comment

Choose a reason for hiding this comment

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

@Dindihub thanks so much for taking the time to submit this PR 🙏🏼

I left some feedback.

In additional:

  • please update description, you need to explicitly write Resolves ISSUE_NUMBER to make sure automated closing after merge will work
  • please move these 2 sections up in the structure of the readme, they should be the first sections that user sees

README.md Outdated
@@ -106,3 +106,25 @@ The manual process of creating a new version is to:
]
}
```

## Overview
* This repository contains JSON Schema files for all the versions of AsyncAPI specification. You can link to https://json-schema.org/ for reference.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* This repository contains JSON Schema files for all the versions of AsyncAPI specification. You can link to https://json-schema.org/ for reference.
* This repository contains [JSON Schema](https://json-schema.org/ for reference) files for all the versions of AsyncAPI specification.

README.md Outdated
## Overview
* This repository contains JSON Schema files for all the versions of AsyncAPI specification. You can link to https://json-schema.org/ for reference.
* These JSON Schema files do not reflect 1:1 the specification and shouldn't be treated as specification itself but rather as a tool for things like validation e.t.c
* These JSON Schema files should'nt be used as the only tool for validation of AsyncAPI documents because some rules described in the AsyncAPI specification are not described here.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* These JSON Schema files should'nt be used as the only tool for validation of AsyncAPI documents because some rules described in the AsyncAPI specification are not described here.
* These JSON Schema files shouldn't be used as the only tool for validating AsyncAPI documents because some rules described in the AsyncAPI specification are not described with JSON Schema.

README.md Outdated
* These JSON Schema files should'nt be used as the only tool for validation of AsyncAPI documents because some rules described in the AsyncAPI specification are not described here.

## Custom Validation Needs
* The JSON Schema does not cover all validation cases, and if a user decides to validate AsyncAPI tools with only JSON Schema provided in this repo, they risk having the AsyncAPI documents not working in tandem with all AsyncAPI tools and this will affect validation.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* The JSON Schema does not cover all validation cases, and if a user decides to validate AsyncAPI tools with only JSON Schema provided in this repo, they risk having the AsyncAPI documents not working in tandem with all AsyncAPI tools and this will affect validation.
* The JSON Schema does not cover all validation cases, and if you decide to validate AsyncAPI tools with only JSON Schema provided in this repo, you risk having the AsyncAPI documents not working in tandem with all AsyncAPI tools, and this will affect validation.

README.md Outdated

## Custom Validation Needs
* The JSON Schema does not cover all validation cases, and if a user decides to validate AsyncAPI tools with only JSON Schema provided in this repo, they risk having the AsyncAPI documents not working in tandem with all AsyncAPI tools and this will affect validation.
* It's recommended to use the custom validation tool provided at https://github.com/asyncapi/parser-js as we look on implementing a comprehensive validation tool.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* It's recommended to use the custom validation tool provided at https://github.com/asyncapi/parser-js as we look on implementing a comprehensive validation tool.
* It's recommended to use [AsyncAPI JavaScript Parser](https://github.com/asyncapi/parser-js) that not only uses AsyncAPI JSON Schema file for validation but also implements additional custom validations.

README.md Outdated
6. Validate if parameters specified in the channel name have corresponding parameters object defined and if name does not contain url parameters.
7. Validate if all servers listed for a channel in servers property are declared in the top-level servers object.
8. Validate if tags specified in the objects have no duplicates. Check is done for: root, operations, operation traits, channels, messages and message traits.
* At the moment, Parser-js do not cover all validation cases yet,all test cases and parsers coverage can be found here -> https://asyncapi.github.io/tck/
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
* At the moment, Parser-js do not cover all validation cases yet,all test cases and parsers coverage can be found here -> https://asyncapi.github.io/tck/
* At the moment, AsyncAPI JavaScript parser do not cover all validation cases yet
* All test cases and parsers coverage can be found [here](https://asyncapi.github.io/tck/)

README.md Outdated
* It's recommended to use the custom validation tool provided at https://github.com/asyncapi/parser-js as we look on implementing a comprehensive validation tool.

* The following additional custom validations need to be provided:
1. Validate if variables provided in the url property have corresponding variable object defined and if the example is correct
Copy link
Member

Choose a reason for hiding this comment

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

this should be the bullet list as the order doesn't matter.

ordering by number should be used only if the order makes a difference

README.md Outdated
8. Validate if tags specified in the objects have no duplicates. Check is done for: root, operations, operation traits, channels, messages and message traits.
* At the moment, Parser-js do not cover all validation cases yet,all test cases and parsers coverage can be found here -> https://asyncapi.github.io/tck/

All names of objects/properties can be double-checked with https://www.asyncapi.com/docs/reference/specification/v2.4.0
Copy link
Member

Choose a reason for hiding this comment

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

this was just a note for you in my input, so when you write this readme doc, you use proper names, and put them in code and capitalize them when needed. This is why in my input I for example wrote operationId and not operationId. It matters a lot in tech content, because it indicates that it is not just a word to describe something but actually technical term used in technology you describe.

I hope that makes sense 😅 let me know if you have some doubts
please address this type of highlighting in the list of custom validations

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted. Let me review and make a new PR.

README.md Outdated
Comment on lines 29 to 30
* At the moment, AsyncAPI JavaScript parser do not cover all validation cases yet
* All test cases and parsers coverage can be found [here](https://asyncapi.github.io/tck/)
Copy link
Member

Choose a reason for hiding this comment

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

this should not be part of bullet list of custom validations cases. There should be a separate paragraph below the bullet list

README.md Outdated
![npm](https://img.shields.io/npm/v/@asyncapi/specs?style=for-the-badge) ![npm](https://img.shields.io/npm/dt/@asyncapi/specs?style=for-the-badge)

> If you are currently using version 2, check out [migration guideline to version 3](./migrations/Migrate%20to%20version%203.md). You might be able to update it without any change.
# AsyncAPI

This is a mono repository, which provides all the JSON Schema documents for validating AsyncAPI documents.

## Overview
* This repository contains [JSON Schema](https://json-schema.org/ for reference) files for all the versions of AsyncAPI specification.
Copy link
Member

Choose a reason for hiding this comment

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

I always recommend to check the preview of the markdown and trying to click links, this one is a broken link

Suggested change
* This repository contains [JSON Schema](https://json-schema.org/ for reference) files for all the versions of AsyncAPI specification.
* This repository contains [JSON Schema](https://json-schema.org) files for all the versions of AsyncAPI specification.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Noted.Let me check on the suggestions. Thanks

README.md Show resolved Hide resolved
Copy link
Member

@derberg derberg left a comment

Choose a reason for hiding this comment

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

@Dindihub I made some final comments

@dalelane @fmvilas any concerns with the message that we pass to the user?

Copy link
Member

@derberg derberg left a comment

Choose a reason for hiding this comment

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

I left some additional comments,

I think one problem here is that you make changes only on local, but sometimes it is easier to do it directly in PR when I prepare for you exact solution

have a look at https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/incorporating-feedback-in-your-pull-request

@@ -106,3 +133,6 @@ The manual process of creating a new version is to:
]
}
```

Copy link
Member

Choose a reason for hiding this comment

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

some not needed empty spaces

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi. I have worked on the changes suggested. And thanks for the reference Doc. It helped. Please review.

README.md Outdated
@@ -1,10 +1,37 @@

Copy link
Member

Choose a reason for hiding this comment

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

some not needed empty spaces

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated
* Validate if parameters specified in the channel name have corresponding parameters object defined and if name does not contain url parameters.
* Validate if all servers listed for a channel in servers property are declared in the top-level servers object.
* Validate if tags specified in the objects have no duplicates. Check is done for: root, operations, operation traits, channels, messages and message traits.
* At the moment, AsyncAPI JavaScript parser do not cover all validation cases yet
Copy link
Member

Choose a reason for hiding this comment

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

this also should not be a bullet list element as it is not one of custom validations that user have to provide

Copy link
Member

Choose a reason for hiding this comment

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

@Dindihub I think you missed this comment, line 24 should not be part of bullet list

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done. Sorry missed that one. Thnaks

Dindihub and others added 4 commits July 25, 2022 21:12
Co-authored-by: Lukasz Gornicki <lpgornicki@gmail.com>
Co-authored-by: Lukasz Gornicki <lpgornicki@gmail.com>
Changed line 24 from bullet ist to paragraph.
derberg
derberg previously approved these changes Jul 28, 2022
Copy link
Member

@derberg derberg left a comment

Choose a reason for hiding this comment

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

@fmvilas @dalelane

Anything from your side. For me it is ready to merge, but I want to make sure you are ok with the messaging we share here.

I'm out next week, so feel free to merge if you also approve folks

Copy link
Member

@fmvilas fmvilas left a comment

Choose a reason for hiding this comment

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

Left some comments, hope they help!

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Dindihub and others added 4 commits July 29, 2022 09:57
Co-authored-by: Fran Méndez <fmvilas@gmail.com>
Co-authored-by: Fran Méndez <fmvilas@gmail.com>
Co-authored-by: Fran Méndez <fmvilas@gmail.com>
Co-authored-by: Fran Méndez <fmvilas@gmail.com>
Dindihub and others added 8 commits July 29, 2022 10:00
Co-authored-by: Fran Méndez <fmvilas@gmail.com>
Co-authored-by: Fran Méndez <fmvilas@gmail.com>
Co-authored-by: Fran Méndez <fmvilas@gmail.com>
Co-authored-by: Fran Méndez <fmvilas@gmail.com>
Co-authored-by: Fran Méndez <fmvilas@gmail.com>
Co-authored-by: Fran Méndez <fmvilas@gmail.com>
Co-authored-by: Fran Méndez <fmvilas@gmail.com>
Co-authored-by: Fran Méndez <fmvilas@gmail.com>
@Dindihub
Copy link
Contributor Author

Left some comments, hope they help!

I have committed the new suggestions. Thanks for the input

@fmvilas fmvilas requested a review from derberg July 29, 2022 07:08
Copy link
Member

@fmvilas fmvilas left a comment

Choose a reason for hiding this comment

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

LGTM 👍

Requesting another review from @derberg since there were lots of changes.

@Dindihub
Copy link
Contributor Author

LGTM +1

Requesting another review from @derberg since there were lots of changes.

Sure

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.

This looks good to me - adds useful clarifications

@sonarqubecloud
Copy link

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

Copy link
Member

@derberg derberg left a comment

Choose a reason for hiding this comment

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

Thanks so much @Dindihub 🙇🏼

@derberg
Copy link
Member

derberg commented Jul 29, 2022

/rtm

@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 3.2.0-next-major-spec.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 3.2.0-next-spec.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 3.2.0 🎉

The release is available on:

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.

5 participants