-
-
Notifications
You must be signed in to change notification settings - Fork 765
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: manual tools added to the Tools Dashboard #1191
Conversation
Signed-off-by: akshatnema <20bcs022@iiitdmj.ac.in>
Signed-off-by: akshatnema <20bcs022@iiitdmj.ac.in>
✅ Deploy Preview for asyncapi-website ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify site settings. |
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-1191--asyncapi-website.netlify.app/ |
Signed-off-by: akshatnema <20bcs022@iiitdmj.ac.in>
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.
@derberg There are various issues to be discussed related to the schema. Some of them are present in the comments below, here is one important one.
We can't provide enum
to the fields
When we provide enum
to a specific field in schema, it restricts the user to use only those specific values that are present in enum
variable as array. This, in my opinion, is wrong according to the functionality available with us in the backend. Even if we get values in the tool file which are not predefined in our list, still we have ways to show the Tool in the Dashboard. Using the variable enum
, we are restricting users to use those specific values only and not their defined/customizable ones. Here's a preview of what error we are facing right now via enum
variable:
The values in the field are not only judged via spelling, but also on the case in which the value is written. Even Javascript
has been rejected due to the enum
specified as JavaScript
which is wrong according to our defined approach. We have already defined Fuse.js
to detect these changes in the spelling and case of the values given by user.
Signed-off-by: akshatnema <20bcs022@iiitdmj.ac.in>
Signed-off-by: akshatnema <20bcs022@iiitdmj.ac.in>
Signed-off-by: akshatnema <20bcs022@iiitdmj.ac.in>
@akshatnema Please also fix the example for categories website/scripts/tools/tools-schema.json Line 109 in 6ef8964
|
Signed-off-by: akshatnema <20bcs022@iiitdmj.ac.in>
I cannot "unclick" the buttons |
Signed-off-by: akshatnema <20bcs022@iiitdmj.ac.in>
config/tools-manual.json
Outdated
"title": "AsyncAPI-format", | ||
"description": "Format an AsyncAPI document by ordering, casing, formatting, and filtering fields.", | ||
"links": { | ||
"repoUrl": "https://github.com/asyncapi/converter-go" |
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 don't think it is correct link
should be https://github.com/thim81/asyncapi-format
@akshatnema modify "language": {
"type": "string",
"description": "Provide information on how other can integrate with the tool in case it is possible. If your code generator generates Python code, then specify Python, even if code generator itself is written in JavaScript. If we forgot about some language then please add it through a pull request to AsyncAPI website repository.",
"anyOf" : [
{
"type": "string",
"enum": [
"Go",
"Java",
"JavaScript",
"HTML",
"C/C++",
"C#",
"Python",
"TypeScript",
"Kotlin",
"Scala",
"Markdown",
"YAML",
"R",
"Rubby",
"Rust",
"Shell",
"Groovy"
]
},
{
"type": "string"
}
]
} This way, valid will be both:
and
but autocompletion should still work. @magicmatatjahu any objections mister? |
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.
all manual tools reviewed
}, | ||
"filters": { | ||
"language": "Java", | ||
"technology": ["Kotlin", "Maven"], |
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.
don't want to be a party pooper, but Kotlin
is a programming language and looks like we have a case where we can have 2 languages. Like in general in case of JS/TS.
technically it should not be that hard to support. We can support languages
as both, string and array, so there is no need to update existing .asyncapi-tool
files 🤔
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.
So should I change the type of language
to both string and array of string in schema? It can take some major changes because in this case, we need to change both backend functions and frontend implementation, including Filters and Contexts.
I added Kotlin as same logic you provided for TypeScript
once, so I thought that Kotlin
can be added in the same way. Also, you can add any dynamic/custom technology you want, there is no issue regarding the matching of technologies with a defined list of technologies.
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 it can be easily handled as a follow-up task. I'm also not 100% sure. I'm always confused what people want in case of JS.
- If tool is written in JS, it can be used in any library in JS or TS
- If tool is written in TS, it can be used in any library in JS or TS
I have no idea if developers simply filter by default both JS and TS or not 😄
So yeah, not critical, can be tackled in a separate issue
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, let's create an issue regarding this and have a chat with other contributors too to have an opinion on whether a language should be a string or an array of strings. Many of them will have different notions and ideologies. Let's have a discussion regarding this on a issue 😅
Co-authored-by: Lukasz Gornicki <lpgornicki@gmail.com>
Signed-off-by: akshatnema <20bcs022@iiitdmj.ac.in>
@derberg changes done according to the review. I've left some comments on the review for the discussion. |
@magicmatatjahu anything from your side? |
Signed-off-by: akshatnema <20bcs022@iiitdmj.ac.in>
Yes, we can. What if I do |
Signed-off-by: akshatnema <20bcs022@iiitdmj.ac.in>
@derberg @magicmatatjahu All stuff done 😅. Previous tools which were removed, are added again. |
Signed-off-by: akshatnema <20bcs022@iiitdmj.ac.in>
Signed-off-by: akshatnema <20bcs022@iiitdmj.ac.in>
@magicmatatjahu can you please again approve my PR as I mistakenly pushed my Github Token? 😅 |
@akshatnema You should remove that token from your account :) |
@akshatnema one one thing missing, imho you should remove this section entirely so there is no duplication of content |
Tbh, I want to take the opinion of @alequetzalli first, whether we should remove this section from docs or not? because I think this part right now belongs to the documentation and we are still in the phase of the first release of our webpage. We shouldn't go with removing the page used by viewers at one go. Instead, I will prefer to first publicize and promote the Tools Dashboard so that users will aware of them. With some more improvements in the Dashboard, we can soon remove the docs tool page. |
Oh interesting! ok... First, I agree with @akshatnema that removing documentation is not related to building this Tool Dashboard. (IOW, it is not duplicate content.) Second, I also agree that we have yet to publicize and promote the Tools Dashboard, so this is not the time to try and delete the tools list/info from Docs. We would need to plan how and why it may make sense to remove that tool list section from docs. Third, I think we should open a Docs GH issue once this dashboard is LIVE that adds a mention of the new dashboard from our Docs. I would add it to the Tools |
https://github.com/asyncapi/website/blob/master/pages/docs/tools/index.md#asyncapi-tools-list is a 1:1 duplication of what we will have in https://asyncapi.com/tools after we merge this PR. We should remove this duplication as according to https://github.com/asyncapi/community/blob/master/new-tool-documentation.md, community members will no longer add/remove stuff in https://github.com/asyncapi/website/blob/master/pages/docs/tools/index.md#asyncapi-tools-list Yes, "just remove" is not good if people are used to the old view. We should put something in exchange and promote it more. @akshatnema please open a separate issue for this. I'm ok to merge this without cleanup of https://github.com/asyncapi/website/blob/master/pages/docs/tools/index.md#asyncapi-tools-list, but we need a follow-up issue in case people are confused, so we can share link. |
/rtm |
Description
This is a follow-up PR for PR #940 for the addition of tools manually to the Tools Dashboard. This will contain all the changes related to the migration of
/docs/tools
page to the Tools Dashboard viamanual-tools.json
file.