[Proposal/Discussion/WIP] Use ESLint and Stylelint to check for potential errors and enforce a consistent coding style - #69
[Proposal/Discussion/WIP] Use ESLint and Stylelint to check for potential errors and enforce a consistent coding style#69AnSq wants to merge 7 commits into
Conversation
| } | ||
|
|
||
| function createChecklistItem(task) { | ||
| function createChecklistItem(task) { /* eslint-disable-line complexity, max-lines-per-function */ |
There was a problem hiding this comment.
There's a bunch of places where I've enabled a complexity rule (like complexity, max-lines-per-function, or max-depth) and then ignored all the violations. The rule is enabled so that the writer of the code is warned that the function they're writing might be getting out of hand and it might be time to refactor something. If that's deemed infeasible or not worth it though, then the eslint-disable comment warns the reader of the code of the potential difficulty in wrapping their brain around it, but assures them that the writer is at least aware of the issue.
| if (task.id.startsWith("daily_")) { when = "today"; } /* eslint-disable-line @stylistic/brace-style */ | ||
| else if (task.id.startsWith("weekly_")) { when = "this week"; } |
There was a problem hiding this comment.
example of the trouble with brace-style
| } | ||
|
|
||
| function updateIncompleteSubtaskCount(task, queryFrom=document) { | ||
| function updateIncompleteSubtaskCount(task, queryFrom = document) { |
There was a problem hiding this comment.
I actually prefer no spaces here, but @stylistic/space-infix-ops doesn't have an option for "ignore default parameters", and that's a sacrifice I'm willing to make.
| #more-info { | ||
| p:not(:last-child):not(:has(+ :is(ul, table))), table { | ||
| p:not(:last-child):not(:has(+ :is(ul, table))), table { /* eslint-disable-line css/no-invalid-properties -- eslint bug workaround */ |
There was a problem hiding this comment.
eslint has trouble parsing some nested CSS
|
|
||
| const now = new Date(); | ||
| const taskTimes = calcTaskTimes(task, now); | ||
| const cycleNumber = calcCycleNumber(task, now); |
There was a problem hiding this comment.
Wow, look at that! It already caught an error!
(I accidentally deleted the definition of cycleNumber in a previous commit.)
|
I had not heard of ESLint before, I will check this out this weekend, but so far looks promising. I like the idea of having a consistent coding style for posterity. |
|
I added Stylelint for checking CSS. It has better support and more options for CSS than ESLint. Right now it's set up to use both ESLint and Stylelint on CSS files, which seems like maybe a bad idea, but they haven't seemed to clash too much so 🤷. The biggest changes here are enforcing modern color function notation and percentages for alpha values. The Stylelint config is also a work in progress. |
| input[type="checkbox"] { | ||
| appearance: none; -webkit-appearance: none; -moz-appearance: none; |
There was a problem hiding this comment.
appearance: none is widely supported without a vendor prefix
AnSq
changed the title
2 participants
AI Disclosure:
ESLint is a static code analysis tool that can catch common JavaScript problems and enforce a consistent coding style. I propose that we adopt it for this project. This PR contains my work so far in setting it up. It is currently a draft while I gather feedback, fine tune the rule settings, and integrate it into our workflow.
Proposed Coding Style
JavaScript
ifandelseblock each have exactly one statement. Unfortunately, the eslint rule doesn't seem to support this. I could maybe write my own, but that seems like a bunch of extra work for not a lot of benefit. Just use/* eslint-disable-line @stylistic/brace-style */for these cases.There are many other rules active from the recommended configuration, but most of them are fairly obvious or have little-to-no effect on our codebase because we're already following them or don't use those language features.
CSS
!important. (None of our uses of!importantseemed necessary, so I removed them all anyway.)JSON
Integration
I plan to set up a GitHub Action to automatically run eslint on push and on pull request, much like we already have for vitest. I may also write a pre-commit hook to do it for you locally.
For now, once you have checked out this branch and run
npm install, you can runnpx eslintto run the checks. There's also an extension for VS Code and vscodium and compatible editors to highlight problems directly in your editor.