-
-
Notifications
You must be signed in to change notification settings - Fork 53
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
ci: added code linting #473
Conversation
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.
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.
@Min2who please update the title of the PR according to the semantic-release prefixes (as shown in the checks). In this case, the prefix is |
@smoya sorry to bother but the workflow requires your approval |
package.json
Outdated
@@ -11,7 +11,8 @@ | |||
"prepublishOnly": "npm run build", | |||
"bundle": "cd tools/bundler && npm i && npm run bundle", | |||
"startNewVersion": "newVersion=$npm_config_new_version node scripts/add-new-version.js", | |||
"lint": "echo 'No linter integrated yet'", | |||
"lint": "eslint --max-warnings 0 --config .eslintrs .", |
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.
"lint": "eslint --max-warnings 0 --config .eslintrs .", | |
"lint": "eslint --max-warnings 0 --config .eslintrc .", |
I think this is why the workflow failed earlier, @Min2who please fix this. The file name is not matched correctly
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.
Resolved with da9d4ce
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
package.json
Outdated
@@ -11,7 +11,8 @@ | |||
"prepublishOnly": "npm run build", | |||
"bundle": "cd tools/bundler && npm i && npm run bundle", | |||
"startNewVersion": "newVersion=$npm_config_new_version node scripts/add-new-version.js", | |||
"lint": "echo 'No linter integrated yet'", | |||
"lint": "eslint --max-warnings 0 --config .eslintrc .", | |||
"lint:fix": "eslint --max-warnings 0 --config . --fix", |
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.
"lint:fix": "eslint --max-warnings 0 --config . --fix", | |
"lint:fix": "eslint --max-warnings 0 --config .eslintrc . --fix", |
@Min2who This should be as given here
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
@Min2who Can this be turned in to a final PR? Looks ok to me. |
@smoya done |
@Min2who PR Testing workflow is failing https://github.com/asyncapi/spec-json-schemas/actions/runs/7250263375/job/19767638609?pr=473#step:11:31 |
The tests are now failing because of linting issues 👍 |
@smoya if i dont add comment disabling the eslint error it fails npm run lint and if i do as in this case it is failing sonarcloud code analysis what should i do |
Hey @smoya @Min2who 👋 After running
@smoya currently I'm not having that much knowledge to debug this 😅 . You'd know better as you've knowledge of the codebase and how things are working internally, so would require you help here... |
.eslintrc
Outdated
- "test/**" | ||
- "*.spec.js" | ||
- "*.test.js" |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
package.json
Outdated
@@ -11,7 +11,8 @@ | |||
"prepublishOnly": "npm run build", | |||
"bundle": "cd tools/bundler && npm i && npm run bundle", | |||
"startNewVersion": "newVersion=$npm_config_new_version node scripts/add-new-version.js", | |||
"lint": "echo 'No linter integrated yet'", | |||
"lint": "eslint --max-warnings 0 --config .eslintrc .", | |||
"lint:fix": "eslint --max-warnings 0 --config . --fix", |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
package.json
Outdated
@@ -11,7 +11,8 @@ | |||
"prepublishOnly": "npm run build", | |||
"bundle": "cd tools/bundler && npm i && npm run bundle", | |||
"startNewVersion": "newVersion=$npm_config_new_version node scripts/add-new-version.js", | |||
"lint": "echo 'No linter integrated yet'", | |||
"lint": "eslint --max-warnings 0 --config .eslintrc .", | |||
"lint:fix": "eslint --max-warnings 0 --config . --fix", |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
package.json
Outdated
@@ -11,7 +11,8 @@ | |||
"prepublishOnly": "npm run build", | |||
"bundle": "cd tools/bundler && npm i && npm run bundle", | |||
"startNewVersion": "newVersion=$npm_config_new_version node scripts/add-new-version.js", | |||
"lint": "echo 'No linter integrated yet'", | |||
"lint": "eslint --max-warnings 0 --config .eslintrs .", |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
.eslintrc.yml
Outdated
@@ -0,0 +1,112 @@ | |||
parser: "@typescript-eslint/parser" |
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.
@Min2who is this needed here? I guess TypeScript and related technologies are not to be used in this 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.
i found the typescript parser to be best among the babel and ecma parser
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 agree this feels unexpected. We are not using Typescript (yet) in this project, so I expect all JS files to fulfill with standard vanilla JS.
Unless there is a strong reason to use this, please remove this line.
scripts/validate-schemas.js
Outdated
@@ -7,82 +7,77 @@ const ajvDraft04 = new AjvDraft04(); | |||
const Ajv = require('ajv'); | |||
const ajv = new Ajv(); | |||
|
|||
function validation (excludedFiles){ | |||
|
|||
/* eslint-disable sonarjs/cognitive-complexity */ |
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.
@AnimeshKumar923 As you recently created this script, would you be happy improving it by reducing the cognitive complexity Sonarcloud complains at this moment? I guess it is a matter of simplifying the script.
If so, let's create a new issue for it.
It seems it's no longer giving the error. But a different error this time which is mentioned here in the 2nd screenshot: #473 (comment) |
If we use |
But that error is expected. We should fix the code and not hiding it by using another eslint parser. |
I think we should create a new issue and proceed to resolve it and then after resolution the linting would pass. Thoughts? |
- plugin:sonarjs/recommended | ||
|
||
parserOptions: | ||
requireConfigFile: false |
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.
Needed to make it work with async
requireConfigFile: false | |
requireConfigFile: false | |
ecmaVersion: 2018 |
scripts/add-new-version.js
Outdated
@@ -42,29 +42,29 @@ function addNewSchemaVersion(newVersion, newVersionDir, latestVersion) { | |||
const obj = require(objFile); | |||
|
|||
// Adapt all the MUST supported schema formats | |||
let mustSupportedSchemaFormats = [] = obj?.else?.properties?.schemaFormat?.anyOf[1]?.enum; | |||
let mustSupportedSchemaFormats = obj?.else?.properties?.schemaFormat?.anyOf[1]?.enum || []; |
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.
Ugly, but i can't come with another solution compatible with this version of ES.
You will need to apply the same approach for the rest.
let mustSupportedSchemaFormats = obj?.else?.properties?.schemaFormat?.anyOf[1]?.enum || []; | |
let mustSupportedSchemaFormats = obj && obj.else && obj.else.properties && obj.else.properties.schemaFormat && obj.else.properties.schemaFormat.anyOf && obj.else.properties.schemaFormat.anyOf[1] && obj.else.properties.schemaFormat.anyOf[1].enum ? obj.else.properties.schemaFormat.anyOf[1].enum : []; |
I don't mind if done here or outside. I dropped a suggestion on how to fix it (ugly but 🤷 ) https://github.com/asyncapi/spec-json-schemas/pull/473/files#r1449018980 |
If it's not that significant of a change (assuming it won't stretch out too far if we implement it), I think it would be okay do it in this PR itself. 👍 |
@AnimeshKumar923 doing this change in this pr only after completing #482 |
@Min2who I think you can start working on this one now as #482 got resolved. |
done @AnimeshKumar923 👍 |
Quality Gate passedThe SonarCloud Quality Gate passed, but some issues were introduced. 2 New issues |
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.
LGTM! 🚀🌔
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 good to me
The impact of the "no-unsafe-optional-chaining" rule was an interesting one to me, I hadn't come across that before, because at first glance I was going to say this makes the code less readable - but having read the docs for the lint rule, I see now why you've done this.
/rtm |
🎉 This PR is included in version 6.4.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Description
this pr introduces linting in the repo
Related issue(s)
fix #467