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

chore(website): configure eslint config similiar to asyncapi website #2031

Merged
merged 11 commits into from
Jun 29, 2024

Conversation

jerensl
Copy link
Contributor

@jerensl jerensl commented Jun 10, 2024

Description

Added eslint automatic fix for modelina-website and added modelina-website to eslint ignore in parent project, so no conflicting rule in the future when we start migrating the same rule used in async website as mentioned here

Related Issue

This PR is being made as mentioned here

Copy link

netlify bot commented Jun 10, 2024

Deploy Preview for modelina ready!

Name Link
🔨 Latest commit aa61921
🔍 Latest deploy log https://app.netlify.com/sites/modelina/deploys/667cc56ed451790008f392bb
😎 Deploy Preview https://deploy-preview-2031--modelina.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

Copy link

sonarcloud bot commented Jun 10, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
0.0% Duplication on New Code

See analysis details on SonarCloud

Copy link
Member

@devilkiller-ag devilkiller-ag left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jerensl, you can also add the eslint and prettier rules from the asyncapi website in this PR and update the PR title accordingly.

@coveralls
Copy link

coveralls commented Jun 11, 2024

Pull Request Test Coverage Report for Build 9448551980

Warning: This coverage report may be inaccurate.

This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 92.303%

Totals Coverage Status
Change from base Build 9382165682: 0.0%
Covered Lines: 5996
Relevant Lines: 6327

💛 - Coveralls

@jerensl
Copy link
Contributor Author

jerensl commented Jun 12, 2024

@jerensl, you can also add the eslint and prettier rules from the asyncapi website in this PR and update the PR title accordingly.

@devilkiller-ag After doing some work I figured out this doesn't work because the next cli makes us dirty here, basically next lint did something different with eslint, it doesn't allow us to have multiple configurations for eslint and always counted the root directory project eslint rule as mentioned in this NextJS discussion, this resulted of conflicting rule with the root directory project.

For the solution, I think it would be better to migrate the website as new repositories and use submodules for this repository

@jonaslagoni what is your opinion about this?

@jonaslagoni
Copy link
Member

For the solution, I think it would be better to migrate the website as new repositories and use submodules for this repository

At the moment that wont be an option, too complex of a setup for now.

@devilkiller-ag After doing some work I figured out this doesn't work because the next cli makes us dirty here, basically next lint did something different with eslint, it doesn't allow us to have multiple configurations for eslint and always counted the root directory project eslint rule as mentioned in this vercel/next.js#36440, this resulted of conflicting rule with the root directory project.

It should be possible to create a website-only eslint configuration even through the next cli, if the next cli is even needed to lint the files.

@jerensl jerensl changed the title chore(website): add nextjs eslint automatic fix on website chore(website): configure eslint config similiar to asyncapi website Jun 18, 2024
@jerensl
Copy link
Contributor Author

jerensl commented Jun 19, 2024

@devilkiller-ag, I made adjusted to ignore next/lint and use only eslint for modelina-website. The next step is to run the fix command, I tested it on my local device, which changes all over the code in the website and leftover ~100(mostly minor changes) errors that need to be fixed manually

@devilkiller-ag
Copy link
Member

devilkiller-ag commented Jun 19, 2024

@devilkiller-ag, I made adjusted to ignore next/lint and use only eslint for modelina-website. The next step is to run the fix command, I tested it on my local device, which changes all over the code in the website and leftover ~100(mostly minor changes) errors that need to be fixed manually

@jerensl what type of errors are those?

@jerensl
Copy link
Contributor Author

jerensl commented Jun 19, 2024

@devilkiller-ag, I made adjusted to ignore next/lint and use only eslint for modelina-website. The next step is to run the fix command, I tested it on my local device, which changes all over the code in the website and leftover ~100(mostly minor changes) errors that need to be fixed manually

@jerensl what type of errors are those?

Mostly unused variable and hoisting

Terminal.mp4

@jerensl
Copy link
Contributor Author

jerensl commented Jun 20, 2024

Hi @devilkiller-ag if you don't mind I changed the entire website codebase, I made the latest commit for fixing around 100 eslint errors, which left around ~90 warnings related to jsdoc for documentation purposes

@devilkiller-ag
Copy link
Member

Hi @devilkiller-ag if you don't mind I changed the entire website codebase, I made the latest commit for fixing around 100 eslint errors, which left around ~90 warnings related to jsdoc for documentation purposes

Okay, No problem!

@coveralls
Copy link

coveralls commented Jun 22, 2024

Pull Request Test Coverage Report for Build 9626634076

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 92.304%

Totals Coverage Status
Change from base Build 9611843317: 0.0%
Covered Lines: 5997
Relevant Lines: 6328

💛 - Coveralls

@coveralls
Copy link

coveralls commented Jun 22, 2024

Pull Request Test Coverage Report for Build 9627168389

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 92.304%

Totals Coverage Status
Change from base Build 9626689374: 0.0%
Covered Lines: 5997
Relevant Lines: 6328

💛 - Coveralls

@jerensl
Copy link
Contributor Author

jerensl commented Jun 23, 2024

@devilkiller-ag, I want to mention here too, with the deprecating of the majority eslint formatting rule, It's been clear they're not buying into the idea of becoming both formatter and linter simultaneously. So I propose a solution to make separation between both tools which we are using prettier for formatter and eslint as a linter

https://eslint.org/blog/2023/10/deprecating-formatting-rules/

@devilkiller-ag
Copy link
Member

@devilkiller-ag, I want to mention here too, with the deprecating of the majority eslint formatting rule, It's been clear they're not buying into the idea of becoming both formatter and linter simultaneously. So I propose a solution to make separation between both tools which we are using prettier for formatter and eslint as a linter

https://eslint.org/blog/2023/10/deprecating-formatting-rules/

Yeah this seems important to me. We should have this separation. Would you like to work on this @jerensl?

@coveralls
Copy link

coveralls commented Jun 25, 2024

Pull Request Test Coverage Report for Build 9657860438

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 92.304%

Totals Coverage Status
Change from base Build 9642600004: 0.0%
Covered Lines: 5997
Relevant Lines: 6328

💛 - Coveralls

@jerensl
Copy link
Contributor Author

jerensl commented Jun 25, 2024

Yeah this seems important to me. We should have this separation. Would you like to work on this @jerensl?

Sure

@coveralls
Copy link

coveralls commented Jun 27, 2024

Pull Request Test Coverage Report for Build 9689394190

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 92.304%

Totals Coverage Status
Change from base Build 9642600004: 0.0%
Covered Lines: 5997
Relevant Lines: 6328

💛 - Coveralls

@devilkiller-ag
Copy link
Member

How's the work going on @jerensl? Is it ready for review?

@devilkiller-ag
Copy link
Member

/rtm

@asyncapi-bot asyncapi-bot merged commit 1b763f2 into asyncapi:master Jun 29, 2024
15 checks passed
@devilkiller-ag
Copy link
Member

Thanks @jerensl!

@asyncapi-bot
Copy link
Contributor

🎉 This PR is included in version 3.7.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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants