-
-
Notifications
You must be signed in to change notification settings - Fork 169
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: integrate v2 ParserJS in validate command #376
feat: integrate v2 ParserJS in validate command #376
Conversation
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
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.
Looks awesome, one thing to add, when we run the validate command and there is a governance issue it says File ./test/specification.yml and/or referenced documents have governance issues.
but the file is valid, shouldn't it say that the file is valid but have governance issue.
@Souvikns So you wanna see something like:
? |
I use cli to quickly check if the spec I am writing is valid, and yeah this would help |
@derberg ping 😄 |
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.
just left 2 comments
other review will do tomorrow
import { html, json, junit, stylish, teamcity, text, pretty } from '@stoplight/spectral-cli/dist/formatters'; | ||
import { OutputFormat } from '@stoplight/spectral-cli/dist/services/config'; |
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.
it is pretty risky to import these helpers from dist
of the CLI.....breaking changes may hurt us later
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.
It will give you errors during the CLI build that these exports don't exist in the path. There is no other way, and creating these functions from scratch or copying them to our codebase is pointless. There is no good solution here and this is the least invasive.
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.
then we should at least make sure that in package.json we do not allow ^
for spectral-cli dependency
package.json
Outdated
@@ -12,11 +12,12 @@ | |||
"@asyncapi/diff": "^0.4.0", | |||
"@asyncapi/generator": "^1.9.12", | |||
"@asyncapi/modelina": "^0.59.8", | |||
"@asyncapi/parser": "^1.17.0", | |||
"@asyncapi/parser": "^2.0.0-next-major.6", |
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.
and here we have the same problem we have with other PR from Jonas. This will be overwritten with next parser release back to 1.18
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.
left just few comments
scripts/fetch-asyncapi-example.js
Outdated
if (!parsedAsyncAPI.hasServers()) {return '';} | ||
const servers = parsedAsyncAPI.servers(); | ||
return Object.keys(servers).map(server => servers[String(server)].protocol()).join(','); | ||
const listAllProtocolsForFile = (document) => { |
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.
good candidate for Parser API 😉
src/models/SpecificationFile.ts
Outdated
return this.getFilePath() || this.getFileURL(); | ||
} | ||
|
||
toDetails() { |
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.
this one sounds weird, do not play well with what it returns
test/commands/diff.test.ts
Outdated
@@ -6,7 +6,7 @@ describe('diff', () => { | |||
test | |||
.stderr() | |||
.stdout() | |||
.command(['diff', './test/specification.yml', './test/specification.yml', '--format=json']) | |||
.command(['diff', './test/specification.yml', './test/specification.yml', '--format=json', '--no-log-diagnostics']) |
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.
when I run diff
successfully I do not want to see diagnostics by default. When I run diff
I'm interested with diff
, unless files that I run through diff have errors and diff
cannot run because of it
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.
since asyncapi/.github#204 the bump.yml
workflow is not going to be replicated in repositories that do not have get-global-node-release-workflows
. This means that you can now easily update it in parser repository, in the master
branch to make sure regular releases will not revert the version update that you did here. So in parser bump workflow you need to add cli
to list of ignored repos
52fd3ee
to
bb64439
Compare
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
@derberg Ok, suggestions applied! Could you check again? |
@magicmatatjahu can you address code smells? |
@derberg You can check that we cannot address such an issues, because Modelina doesn't support in input parsed AsyncAPI document (but only in types) - in the logic we check that. However, that code is in HEAD on master https://github.com/asyncapi/cli/blob/master/src/commands/generate/models.ts#L164 so that code isn't added by my PR. |
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.
@magicmatatjahu right, thanks for point that out
@jonaslagoni Do you wanna check. I changed some code in models generation? |
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.
@magicmatatjahu not necessary, but now that I am here LGTM 👍
@derberg asyncapi/parser-js#701 and asyncapi/parser-js#702 Could you check? Thanks! |
/rtm |
🎉 This PR is included in version 0.30.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
validate
commandgenerate models
anddiff
commands and determine if command should fail or should perform logic if diagnostics severity is allowed.Example input:
Example output:
Related issue(s)
Part of asyncapi/parser-js#481