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

Fix linter warnings #199

Merged
merged 2 commits into from Jan 1, 2023
Merged

Conversation

croumegous
Copy link
Collaborator

@croumegous croumegous commented Dec 31, 2022

Fixed all warnings in next build/next lint

Added a new npm command: npm run fix to run format and lint code.
To make review easier I have made 2 commits, the first one adds eslint plugins to automatically handle import sort and unused import.
And a second commit which fixes the remaining warning manually, so this commit is more "opinionated" and needs more attention during the review.
(I have added some //eslint-disable-next-line for some part, where it seems to be the best option)

(There is a lot of changes so it might create conflicts for other people working on /website)

Copy link
Collaborator

@AbdBarho AbdBarho left a comment

Choose a reason for hiding this comment

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

@croumegous thanks for the contribution!

could you please rebase? easier to review.

With the exception of my comment it looks fine.

Comment on lines 10 to 12
"simple-import-sort/exports": "error",
"@typescript-eslint/no-unused-vars": "off",
"unused-imports/no-unused-imports": "error",
"unused-imports/no-unused-vars": [
"warn",
{
"vars": "all",
"varsIgnorePattern": "^_",
"args": "after-used",
"argsIgnorePattern": "^_"
}
]
},
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know that customizing eslint is always very opinionated... imo there is no need for these rules.

I will have to wait for opinions of other team member...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I've changed it to keep the less rule as possible to make it work:

"unused-imports/no-unused-imports": "warn",  //mandatory to automatically remove unused import  
"simple-import-sort/imports": "warn", //mandatory to automatically sort import  
"simple-import-sort/exports": "warn"  //mandatory to automatically sort export

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think these three small additions look good.

@yk yk linked an issue Dec 31, 2022 that may be closed by this pull request
@yk yk added the website label Dec 31, 2022
Copy link
Collaborator

@fozziethebeat fozziethebeat left a comment

Choose a reason for hiding this comment

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

Things look a lot cleaner now, thank you~

@fozziethebeat
Copy link
Collaborator

Please fix the merge issues and then I'll re-review

@fozziethebeat fozziethebeat merged commit 41e5094 into LAION-AI:main Jan 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Cleanup all eslint warnings in the website
4 participants