Adding ESLint #12
Adding ESLint #12
Conversation
| "avoidEscape": true | ||
| } | ||
| ], | ||
| "semi": ["error", "never"], |
There was a problem hiding this comment.
Only change I made from my standard ruleset =)
| @@ -22,8 +22,11 @@ | |||
| "@types/chai": "4.3.4", | |||
| "@types/mocha": "10.0.1", | |||
| "@types/node": "18.14.0", | |||
| "@typescript-eslint/eslint-plugin": "^5.56.0", | |||
There was a problem hiding this comment.
What's your opinion on hats (^)? I've never liked them as I am not certain what versions I am using, and I have to trust authors to follow semver.
There was a problem hiding this comment.
Honestly that's why we have package-lock. It's purpose really is exactly this, and also so we can go back later and be really sure we're testing with the version that was checked in.
So package-lock.json gives us stable unchanging dependencies, and package.json gives us information on what we could probably safely upgrade to, but it doesn't happen by default.
| import {downloadAndValidateLedger} from "../../lib/download" | ||
| import {TokenAwareCommand} from "../../lib/token-command" | ||
| import { initClient } from '../../lib/client'; | ||
| import {Flags} from '@oclif/core' |
There was a problem hiding this comment.
' vs " - my take, I use ' for JS files and " for TS as it reminds me of Java (type systems). I am half-seriously thinking of switching to ` for everything, but haven't made that change.
| @@ -27,7 +26,7 @@ describe('ledger:download', async () => { | |||
| expect(req.headers.get('Authorization')).to.equal(`Bearer ${testToken}`) | |||
| const path = new URL(req.url).pathname | |||
| switch(path) { | |||
| case '/ledgers/download' : | |||
| case '/ledgers/download' : { | |||
There was a problem hiding this comment.
Really? That looks so bizarre in a case statement!
There was a problem hiding this comment.
I know why I don't like it. The closure doesn't do anything. One still has to break out of the case. Typescript needs better method pattern matching or something.
There was a problem hiding this comment.
So the reason eslint wants this is a pretty specific reason, it's because const was used inside the body of the 'case'.
I don't love this syntax either but it is one of those bug-preventing things. You may have noticed I only did it for this case, but not for the others (because it's the only one that defines a new variable).
Here's the eslint docs on this rule:
https://eslint.org/docs/latest/rules/no-case-declarations
We can still disable it though, let me know! I super don't care about this rule because the kind of the bug it prevents is pretty subtle.
There was a problem hiding this comment.
Thanks for digging into these rules.
|
Looks good, merge when ready. |
I split this PR in 3 commits:
Happy to make changes to the rules if there's anything you don't like about the style it decided on, and also happy to elaborate on why I made certain choices. Some are obviously stylistic, but there's also a few that help reduce bugs.