Skip to content

[p5.js 2.x] chore: enable eslint rules #7930

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.

Sign up for GitHub

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

Jump to bottom
Draft
wants to merge 5 commits into
base: dev-2.0
Choose a base branch
from
Draft

[p5.js 2.x] chore: enable eslint rules #7930

wants to merge 5 commits into from

Conversation

error-four-o-four
Copy link
Contributor

@error-four-o-four error-four-o-four commented Jun 22, 2025 •
edited
Loading

Hey there,

this PR is a follow-up to #7853 to discuss the usage of linter rules.

It does not include the fixed/applied changes of the rules because it would block other PRs or would result in a lot of rebasing and resolving merge conflicts. I'd argue that the fixes should be applied in separate PRs.

(Discussed rules were marked as completed)

eslint recommended

custom p5

Stylistic

jsdoc

@eslint/markdown

@error-four-o-four
feat: enable stylistic rules
218e428 
`comma-dangele`, `indent`, `linebreak-style`,
`no-trailing-spaces`, `object-curly-spacing`,
`quotes`, `semi`
@error-four-o-four
fix: update @eslint/markdown to version 6.6.0 and enable 'markdown/no…
b880d7a 
…-duplicate-headings' rule
@error-four-o-four
feat: enable jsdoc rules
00c0032 
`jsdoc/check-alignment`, `jsdoc/no-multi-asterisks`
`jsdoc/require-asterisk-prefix`
@error-four-o-four
cleanup: move md rules to the top
0b1e3ea
@limzykenneth
Copy link
Member

An early suggestion, I think in terms of warning vs error I would like it to be so that things that directly lead to unintended behavior or rather just the eslint:recommended list to be error, while the stylistic ones to remain as warnings.

There are a few reasons I'm thinking of, the main one is in terms of cognitive understanding with a code editor linter, errors will be highlighted red which should be immediately actioned while warnings highlighted yellow can be dealt with later before commit. We can set ESLint to exit with non-zero exit code on warning as well to get the CI to fail on warning still I believe. That along with a functional eslint-fix should make code style easy enough to work with. Does that make sense to you or do you have a different idea?

@error-four-o-four
Copy link
Contributor Author

An early suggestion, I think in terms of warning vs error I would like it to be so that things that directly lead to unintended behavior or rather just the eslint:recommended list to be error, while the stylistic ones to remain as warnings.

There are a few reasons I'm thinking of, the main one is in terms of cognitive understanding with a code editor linter, errors will be highlighted red which should be immediately actioned while warnings highlighted yellow can be dealt with later before commit. We can set ESLint to exit with non-zero exit code on warning as well to get the CI to fail on warning still I believe. That along with a functional eslint-fix should make code style easy enough to work with. Does that make sense to you or do you have a different idea?

It makes perfect sense. I enabled the stylistic rules in order to get rid of the noise but that's not the correct intention.

To get the CI to fail we'd have to use --max-warnings 0

Once all the fixes we're applied one could reenable .vscode/settings.json#editor.codeActionsOnSave, right? I suggested to disable this setting in order to not accidentally stage any files (via git add .) which weren't intentionally edited by a contributor.

@limzykenneth
Copy link
Member

I don't use vscode that much, does .vscode/settings.json#editor.codeActionsOnSave mean it will run eslint fix on save on the saved file? If so we can possible reenable it but the main thing to make sure before that is that there won't be any thing else changing the code beyond our eslint rules, ie. vscode won't change other stylistic stuff that may not be directly covered by our eslint rules like changing whitespace after function name if we don't define it.

@error-four-o-four
Copy link
Contributor Author

I don't use vscode that much, does .vscode/settings.json#editor.codeActionsOnSave mean it will run eslint fix on save on the saved file? If so we can possible reenable it but the main thing to make sure before that is that there won't be any thing else changing the code beyond our eslint rules, ie. vscode won't change other stylistic stuff that may not be directly covered by our eslint rules like changing whitespace after function name if we don't define it.

Yep, that's the purpose of ./vscode/settings.json#"javascript.format.enable": false. Vscode made me go insane before this setting was added.

@error-four-o-four error-four-o-four mentioned this pull request Jun 22, 2025
Open
@error-four-o-four error-four-o-four changed the title chore: enable eslint rules [p5.js 2.0] chore: enable eslint rules Jun 22, 2025
@error-four-o-four error-four-o-four changed the title [p5.js 2.0] chore: enable eslint rules [p5.js 2.x] chore: enable eslint rules Jun 22, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@error-four-o-four @limzykenneth