feat: switch to new parser API #349
Conversation
jonaslagoni
requested review from
derberg,
magicmatatjahu,
fmvilas and
asyncapi-bot-eve
as code owners
|
The last two code-smells are not code-smells cause it's unrelated to templates as we don't re-render at any point in time. Dont know how to disable them though, think you have to manually ignore them in the platform. |
|
@jonaslagoni you need to solve some conflicts |
|
@derberg done 👍 |
|
@jonaslagoni left one comment regarding sonar, you manage exclusions through file like https://github.com/asyncapi/parser-js/blob/master/.sonarcloud.properties |
@derberg I stopped trying to figure out exactly what rules you need to use here to disable a single rule on a single line, it's way too complicated. Do you know what the rule should say, exactly? |
|
@jonaslagoni haven't done it is some time but probably you need to add https://github.com/jsx-eslint/eslint-plugin-react/blob/master/docs/rules/no-array-index-key.md rule with value |
|
@derberg does not seem to work no |
|
bolox...then just add entire component to ignore for now https://github.com/asyncapi/markdown-template/blob/master/.sonarcloud.properties#L2 |
|
@derberg done 👍 |
There was a problem hiding this comment.
found one bug, and also not sure why but:
tested with https://raw.githubusercontent.com/asyncapi/asyncapi/2.0.0/examples/2.0.0/streetlights.yml
with current template release, for message turnOnOff I do not see ` info about content type
with this PR, I get text: * Content type: [application/json](https://www.iana.org/assignments/media-types/application/json)
not sure a bug, but still curious why it shows up now 🤔
we are definitely missing snapshot tests. Maybe I will open a separate PR to add it first, so you update this PR accordingly and we see? Cause now it is manual
| @@ -88,8 +91,9 @@ function Operation({ type, asyncapi, operation, channelName, channel }) { // NOS | |||
| <ListItem> | |||
| Available only on servers:{' '} | |||
| {servers.map(s => { | |||
| const slug = FormatHelpers.slugify(s); | |||
| return `[${s}](#${slug}-server)`; | |||
| const serverId = s.id(); | |||
There was a problem hiding this comment.
tested with https://raw.githubusercontent.com/asyncapi/asyncapi/2.0.0/examples/2.0.0/streetlights.yml
with current template release, it is now showing up, as I do not have any info on a channel level that given channel is only or production or somewhere else
with this PR, I get text: * Available only on servers: [production](#production-server) which is not true
There was a problem hiding this comment.
Technically it's true because, without the channel explicitly defining a server, it's every server defined in servers.
There was a problem hiding this comment.
This is the parser-api's doing, because it defaults to all servers, because the channel defines none 🙂
There was a problem hiding this comment.
@derberg question is, do you want it to be the same as before or with this new display?
There was a problem hiding this comment.
but there is just one server, so sentence Available only on servers: production is only confusing, like wait are there any other servers available?. It's like with a warning on a plastic bag to not put it on your head, like why would I even try 😄
There was a problem hiding this comment.
They do it in the US 😆
There was a problem hiding this comment.
yeah, I know 🤣 but markdown-template is not used only in the US 😄
There was a problem hiding this comment.
Alright, changed the implementation to only show the servers the application is operating on when it is NOT all servers 🙂
|
Kudos, SonarCloud Quality Gate passed!
|
wdyt, do you mind to wait until I do it |
|
@derberg what exactly is missing? Full end-to-end tests? Cause @magicmatatjahu did update the tests accordingly as well, although they are not snapshots of course. |
|
We also had a test for #349 (comment), but it was updated to expect a server, which I reverted in beebb69 |
|
/rtm |
|
🎉 This PR is included in version 1.3.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Description
This PR is a continuum of #312, @magicmatatjahu did most of the work, I just finish the last details.
It updates the template to use the new parser API.
Related issue(s)
Fixes #345