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

feat: support yaml as format for .asyncapi-tools #1135

Merged
merged 3 commits into from
Dec 1, 2022

Conversation

derberg
Copy link
Member

@derberg derberg commented Nov 30, 2022

@akshatnema I opened a PR as I remember we agreed that it is of your scope for mentorship.

I decided to add YAML support now, next to JSON because when I was writing first file, it felt better to do it with YAML -> https://github.com/asyncapi/cli/blob/master/.asyncapi-tool

@netlify
Copy link

netlify bot commented Nov 30, 2022

Deploy Preview for asyncapi-website ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit 7f9cad0
🔍 Latest deploy log https://app.netlify.com/sites/asyncapi-website/deploys/638885b613cc8f00084f77bb
😎 Deploy Preview https://deploy-preview-1135--asyncapi-website.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site settings.

{"Code-first tools":{"description":"The following is a list of tools that generate AsyncAPI documents from your code.","toolsList":[]},"Code Generators":{"description":"The following is a list of tools that generate code from an AsyncAPI document; not the other way around.","toolsList":[{"title":"Sample Tool","description":"Tool for testing","links":{"websiteUrl":"https://akshatnema.netlify.app","docsUrl":"","repoUrl":"https://github.com/akshatnema/Login-Registration-project"},"filters":{"language":"javascript","technology":["react"],"categories":["code generator"],"hasCommercial":false,"isAsyncAPIOwner":false}}]},"Converters":{"description":"The following is a list of tools that do not yet belong to any specific category but are also useful for the community.","toolsList":[]},"Directories":{"description":"The following is a list of directories that index public AsyncAPI documents.","toolsList":[]},"Documentation Generators":{"description":"The following is a list of tools that generate human-readable documentation from an AsyncAPI document.","toolsList":[]},"UI components":{"description":"The following is a list of UI components to view AsyncAPI documents.","toolsList":[]},"DSL":{"description":"Writing YAML by hand is no fun, and maybe you don't want a GUI, so use a Domain Specific Language to write AsyncAPI in your language of choice.","toolsList":[]},"Frameworks":{"description":"The following is a list of API/application frameworks that make use of AsyncAPI.","toolsList":[]},"GitHub Actions":{"description":"The following is a list of GitHub Actions that you can use in your workflows","toolsList":[]},"Mocking and Testing":{"description":"The tools below take specification documents as input, then publish fake messages to broker destinations for simulation purposes. They may also check that publisher messages are compliant with schemas.","toolsList":[]},"Validators":{"description":"The following is a list of tools that validate AsyncAPI documents.","toolsList":[]},"Compare tools":{"description":"The following is a list of tools that compare AsyncAPI documents.","toolsList":[]},"Others":{"description":"The following is a list of tools that comes under Other category","toolsList":[{"title":"AsyncAPI CLI","description":"One CLI to rule them all. \nThis is a CLI that aims to integrate all AsyncAPI tools that you need while AsyncAPI document development and maintainance. \nYou can use it to generate docs or code, validate AsyncAPI document and event create new documents.\n","links":{"websiteUrl":"https://www.asyncapi.com/tools/cli","repoUrl":"https://github.com/asyncapi/cli"},"filters":{"technology":["TypeScript"],"categories":["others","cli"],"hasCommercial":false,"isAsyncAPIOwner":true}}]},"CLIs":{"description":"The following is a list of tools that you can work with in terminal or do some CI/CD automation","toolsList":[{"title":"AsyncAPI CLI","description":"One CLI to rule them all. \nThis is a CLI that aims to integrate all AsyncAPI tools that you need while AsyncAPI document development and maintainance. \nYou can use it to generate docs or code, validate AsyncAPI document and event create new documents.\n","links":{"websiteUrl":"https://www.asyncapi.com/tools/cli","repoUrl":"https://github.com/asyncapi/cli"},"filters":{"technology":["TypeScript"],"categories":["others","cli"],"hasCommercial":false,"isAsyncAPIOwner":true}}]}}
Copy link
Member Author

Choose a reason for hiding this comment

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

just an update, CLI added, not much to review

@@ -69,5 +69,9 @@
"Others": {
"description": "The following is a list of tools that comes under Other category",
"toolsList": []
},
"CLIs": {
Copy link
Member Author

Choose a reason for hiding this comment

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

followed approach that all categories, even empty are here in manual file so people that add manually have easier life

{"Code-first tools":{"description":"The following is a list of tools that generate AsyncAPI documents from your code.","toolsList":[]},"Code Generators":{"description":"The following is a list of tools that generate code from an AsyncAPI document; not the other way around.","toolsList":[{"title":"Sample Tool","description":"Tool for testing","links":{"websiteUrl":"https://akshatnema.netlify.app","docsUrl":"","repoUrl":"https://github.com/akshatnema/Login-Registration-project"},"filters":{"language":{"name":"Javascript","color":"#F2F1C7","borderColor":"#BFBE86"},"technology":[],"categories":["code generator"],"hasCommercial":false,"isAsyncAPIOwner":false}}]},"Converters":{"description":"The following is a list of tools that do not yet belong to any specific category but are also useful for the community.","toolsList":[]},"Directories":{"description":"The following is a list of directories that index public AsyncAPI documents.","toolsList":[]},"Documentation Generators":{"description":"The following is a list of tools that generate human-readable documentation from an AsyncAPI document.","toolsList":[{"title":"asyncapi-asciidoc-template","description":"Asciidoc template for the asyncapi generator","links":{"repoUrl":"https://gitlab.com/djencks/asyncapi-asciidoc-template"},"filters":{"language":{"name":"Javascript","color":"#F2F1C7","borderColor":"#BFBE86"},"technology":[],"categories":["documentation generator"],"hasCommercial":false,"isAsyncAPIOwner":false}}]},"UI components":{"description":"The following is a list of UI components to view AsyncAPI documents.","toolsList":[]},"DSL":{"description":"Writing YAML by hand is no fun, and maybe you don't want a GUI, so use a Domain Specific Language to write AsyncAPI in your language of choice.","toolsList":[]},"Frameworks":{"description":"The following is a list of API/application frameworks that make use of AsyncAPI.","toolsList":[]},"GitHub Actions":{"description":"The following is a list of GitHub Actions that you can use in your workflows","toolsList":[]},"Mocking and Testing":{"description":"The tools below take specification documents as input, then publish fake messages to broker destinations for simulation purposes. They may also check that publisher messages are compliant with schemas.","toolsList":[]},"Validators":{"description":"The following is a list of tools that validate AsyncAPI documents.","toolsList":[]},"Compare tools":{"description":"The following is a list of tools that compare AsyncAPI documents.","toolsList":[]},"Others":{"description":"The following is a list of tools that comes under Other category","toolsList":[{"title":"AsyncAPI CLI","description":"One CLI to rule them all. \nThis is a CLI that aims to integrate all AsyncAPI tools that you need while AsyncAPI document development and maintainance. \nYou can use it to generate docs or code, validate AsyncAPI document and event create new documents.\n","links":{"websiteUrl":"https://www.asyncapi.com/tools/cli","repoUrl":"https://github.com/asyncapi/cli"},"filters":{"technology":[],"categories":["others","cli"],"hasCommercial":false,"isAsyncAPIOwner":true}}]},"CLIs":{"description":"The following is a list of tools that you can work with in terminal or do some CI/CD automation","toolsList":[{"title":"AsyncAPI CLI","description":"One CLI to rule them all. \nThis is a CLI that aims to integrate all AsyncAPI tools that you need while AsyncAPI document development and maintainance. \nYou can use it to generate docs or code, validate AsyncAPI document and event create new documents.\n","links":{"websiteUrl":"https://www.asyncapi.com/tools/cli","repoUrl":"https://github.com/asyncapi/cli"},"filters":{"technology":[],"categories":["others","cli"],"hasCommercial":false,"isAsyncAPIOwner":true}}]}}
Copy link
Member Author

Choose a reason for hiding this comment

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

just an update, CLI added, not much to review

@@ -1,18 +1,20 @@
const { getData } = require('./tools/extract-tools-github');
const { convertTools } = require('./tools/tools-object');
const { combineTools } = require('./tools/combine-tools');
const manualTools = require('../config/tools-manual.json')
Copy link
Member Author

Choose a reason for hiding this comment

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

I moved it here from combine-tools as it felt confusing for me that we read one of input data inside the helper function

Copy link
Member Author

Choose a reason for hiding this comment

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

rest of changes is just related to it

@github-actions
Copy link

github-actions bot commented Nov 30, 2022

⚡️ Lighthouse report for the changes in this PR:

Category Score
🔴 Performance 43
🟢 Accessibility 96
🟠 Best practices 83
🟢 SEO 100
🔴 PWA 30

Lighthouse ran on https://deploy-preview-1135--asyncapi-website.netlify.app/

{
name: "CLIs",
tag: "cli",
description: "The following is a list of tools that you can work with in terminal or do some CI/CD automation"
Copy link
Member Author

Choose a reason for hiding this comment

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

came up with new category because there was no better one for AsyncAPI cli

if (languageSearch.length) {
finalObject.filters.language = languageSearch[0].item;

//there might be a tool without language
Copy link
Member Author

Choose a reason for hiding this comment

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

in case of CLIs there is no language

Copy link
Member

Choose a reason for hiding this comment

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

Means that there can be certain tools in list whose language is not defined, right?

@@ -25,7 +25,7 @@
},
"filters": {
"type": "object",
"required": ["language", "technology", "categories"],
Copy link
Member Author

Choose a reason for hiding this comment

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

in case of CLIs there is no language, this is why I made it optional

@@ -2,7 +2,7 @@
"$schema": "http://json-schema.org/draft-07/schema#",
"title": "JSON Schema for AsyncAPI tool discovery file.",
"type": "object",
"required": ["title", "description", "maintainers", "links", "filters"],
Copy link
Member Author

Choose a reason for hiding this comment

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

yeah, maintainers are not used anymore

@derberg
Copy link
Member Author

derberg commented Nov 30, 2022

@akshatnema @magicmatatjahu added few comments to make it easier with review

Copy link
Member

@magicmatatjahu magicmatatjahu left a comment

Choose a reason for hiding this comment

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

Two comments, rest is good 👏🏼

Comment on lines 49 to +56
for (const key in automatedTools) {
let finalToolsList = [];
if (automatedTools[key].toolsList.length) {
for (const tool of automatedTools[key].toolsList) {
finalToolsList.push(await getFinalTool(tool))
}
}
if (manualTools[key].toolsList.length) {
if (manualTools[key] && manualTools[key].toolsList.length) {
Copy link
Member

Choose a reason for hiding this comment

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

What if automatedTools doesn't have key that is in manualTools? 😄 Because in current code we can "skip" some tools defined in the manualTools.

Copy link
Member

Choose a reason for hiding this comment

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

We already have defined certain categories in the category list so if anyone adds a new category in manual-tools, it has to be updated on the category-list file first, because it is connected to frontend implementation also.

Copy link
Member Author

Choose a reason for hiding this comment

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

if something that is not in category list is added to manual list, it is just ignored, which is good

if something is added to manual list, and category list is updated, then it automatically is in automated list

or

tl;dr - there will be no issues. The only reason I extended this if is in case there is something in category list, but was not added to manual list

Copy link
Member

Choose a reason for hiding this comment

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

Couldn't this logic be made more complicated? 😆 But ok, you guys are more familiar with the implementations, so it's ok for me.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I agree it is quite complicated for me also. If don't revise it for months, I'll forget the logic behind this implementation 😅

Comment on lines 46 to 47
for (let tool of dataArray) {

Copy link
Member

Choose a reason for hiding this comment

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

I think that code inside loop should be also wrapped by try catch, only to console log possible errors in code.

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't think it is needed. In case of errors other than validation we should throw error and not console log. And this is happening. In the end conversion function is wrapped in try/catch in place where it is called

Copy link
Member

@magicmatatjahu magicmatatjahu Dec 1, 2022

Choose a reason for hiding this comment

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

In the end conversion function is wrapped in try/catch in place where it is called

Where? I don't see any try/catch. I only see checking errors by validate function, but it's not a typical try/catch.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I agree with @magicmatatjahu, we should have try/catch block here and I got some errors on this part of code only. Basically, we aren't putting any error handling for axios function, so it would be better if we make a try/catch block there to get correct error log messages.

Copy link
Member

Choose a reason for hiding this comment

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

Where? I don't see any try/catch. I only see checking errors by validate function, but it's not a typical try/catch.

I think @derberg is talking about the try/catch block used in scripts/build-tools.js file which applies to every function call made inside the functionality.

Copy link
Member

Choose a reason for hiding this comment

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

It can be fixed in other PR, I don't want to block you guys :)

Copy link
Member

Choose a reason for hiding this comment

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

@akshatnema

I think @derberg is talking about the try/catch block used in scripts/build-tools.js file which applies to every function call made inside the functionality.

Aaa ok, thanks for clarification!

@akshatnema
Copy link
Member

/rtm

@asyncapi-bot asyncapi-bot merged commit b7c252e into asyncapi:master Dec 1, 2022
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