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

Add code linting (eslint) as part of CI script #467

Closed
4 tasks
smoya opened this issue Dec 11, 2023 · 12 comments · Fixed by #473
Closed
4 tasks

Add code linting (eslint) as part of CI script #467

smoya opened this issue Dec 11, 2023 · 12 comments · Fixed by #473
Labels
area/ci-cd Specify what technical area given issue relates to. Its goal is to ease filtering good first issues. enhancement New feature or request good first issue Good for newcomers released

Comments

@smoya
Copy link
Member

smoya commented Dec 11, 2023

Reason/Context

The number of JS files is growing lately, and I feel it's about time to let all these code pass through a code linter in order to ensure consistency and follow standards, also to find and fix potential bugs.

Description

  • Install eslint package as dev dependency
  • Create .eslintrc and .eslintrcignore files based on, for example, parser-js. Be aware this project doesn't use TypeScript, so modification of those files might be needed.
  • Add new scripts. npm lint and npm lint:fix as, for example, parser-js has.
  • Ensure the linter runs on CI (GitHub actions) on each PR.
@smoya smoya added the enhancement New feature or request label Dec 11, 2023
@smoya smoya added good first issue Good for newcomers area/ci-cd Specify what technical area given issue relates to. Its goal is to ease filtering good first issues. labels Dec 11, 2023
@AnimeshKumar923
Copy link
Contributor

AnimeshKumar923 commented Dec 11, 2023

I would like to work on this issue.

@smoya
Copy link
Member Author

smoya commented Dec 11, 2023

I would like to work in this issue.

Of course! Feel free to create a PR at anytime 🚀

@Gmin2
Copy link
Contributor

Gmin2 commented Dec 11, 2023

@AnimeshKumar923 can i go ahead with this

@AnimeshKumar923
Copy link
Contributor

AnimeshKumar923 commented Dec 11, 2023

@smoya I would like to give @Min2who this issue.

@Min2who Please go ahead with this issue 👍

@Gmin2
Copy link
Contributor

Gmin2 commented Dec 11, 2023

@smoya I would like to give @Min2who this issue.

@Min2who Please go ahead with this issue 👍

thnx @AnimeshKumar923

@AnimeshKumar923
Copy link
Contributor

AnimeshKumar923 commented Dec 11, 2023

thnx @AnimeshKumar923

Welcome! Let me know if you encounter any difficulties...

@Gmin2
Copy link
Contributor

Gmin2 commented Dec 17, 2023

@smoya @AnimeshKumar923 how do i ensure this
image

@AnimeshKumar923
Copy link
Contributor

AnimeshKumar923 commented Dec 17, 2023

@smoya @AnimeshKumar923 how do i ensure this image

About this, @smoya If I rememeber correctly, Lukasz talked about modifying the workflows in .github repositories. So should we open a PR modifying the CI in if-nodejs-pr-testing.yml file to include the linting?

Because if we modify in this repo, it will later be overwritten. So to make it permanent, we should follow the above said approach. Thoughts? 👀


context: #452 (review)

@smoya
Copy link
Member Author

smoya commented Dec 17, 2023

@smoya @AnimeshKumar923 how do i ensure this image

About this, @smoya If I rememeber correctly, Lukasz talked about modifying the workflows in .github repositories. So should we open a PR modifying the CI in if-nodejs-pr-testing.yml file to include the linting?

Because if we modify in this repo, it will later be overwritten. So to make it permanent, we should follow the above said approach. Thoughts? 👀


context: #452 (review)

The workflow is already calling the lint script https://github.com/asyncapi/.github/blob/master/.github%2Fworkflows%2Fif-nodejs-pr-testing.yml#L74-L75

@AnimeshKumar923
Copy link
Contributor

AnimeshKumar923 commented Dec 17, 2023

The workflow is already calling the lint script https://github.com/asyncapi/.github/blob/master/.github%2Fworkflows%2Fif-nodejs-pr-testing.yml#L74-L75

Okay, then as I can see in #473, it will be executed after merging.
cc: @Min2who

@Gmin2
Copy link
Contributor

Gmin2 commented Dec 17, 2023

thnx @smoya @AnimeshKumar923 for the info

@asyncapi-bot
Copy link
Contributor

🎉 This issue has been resolved in version 6.4.0 🎉

The release is available on:

Your semantic-release bot 📦🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/ci-cd Specify what technical area given issue relates to. Its goal is to ease filtering good first issues. enhancement New feature or request good first issue Good for newcomers released
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants