Linting: StandardJS for src/, test/; Prettier for types/

[ChristopherChudzicki]

What are the associated issues?

#2488

See #2542, #2541 for earlier efforts

What does this PR do?

This PR switches to use ESLint directly rather than the StandardJS CLI. Roughly, the config is:

// for js (and a variant with 'plugin:mocha/recommended' for js tests)
    extends: ['standard']
// for typescript
    extends: [
        'plugin:@typescript-eslint/recommended',
        'plugin:prettier/recommended'
    ]

This is the approach (tentatively) decided upon in #2542 (See #2542 (comment) and @josdejong's reply thereto). With this approach:

• Everything is enforced through ESLint. So:
  • Good IDE support (e.g., in VSCode, you'd just need the ESLint extension)
  • There's still just npm run lint and npm run lint -- --fix.
• Small diff, because source code and tests still following StandardJS
• Good maintence:
  • StandardJS seems reasonably maintained for JS files And the StandardJS CLI uses ESLint under the hood, so I imagine any updates to StandardJS have to go through their ESLint config, though I'm not 100% sure.
  • Prettier is definitely well maintained
• We can tweak Prettier's config a little bit. It's not very configurable, but we can tell it to use "no semicolons" and "single quotes", which keeps the TS formatting style fairly similar to the JS formatting style.

[ChristopherChudzicki]

@josdejong In discussion of #2542 you mentioned

In another future step we may want to move towards "eslint" defaults and away from "standardjs". But since that will be a huge change breaking all open PR's and branches, let's only do that if there is real need for it and then plan it smartly.

If you ever decide this is worth pursuing, I believe almost all merge conflicts could be resolved nearly automatically through some git surgery which I laid out in #2541 (comment) The 10-step guide is perhaps a bit daunting, but it might be possible to automate those steps. I wrote that mostly as a proof-of-concept.

But a big change like that would also clutter visibility of the timeline in git history. I don't think that's avoidable :/

[gwhitney]

As this is still a fairly big watershed, as before I will leave this to you and Jos. I'll just make a couple comments/observations which you can take or leave.

[gwhitney]

I will be honest I did not go over all of index.d.ts carefully. My eyes glazed over with all of the semicolon removals. Which does beg the question (which should really be addressed separately from this PR) of whether there's any convenient mechanism for index.d.ts and likely index.ts as well to be split into multiple files, maybe one per src/ subdirectory or something like that.

Anyhow, my overall takeaway is that this is a reasonable approach. And now as I said I will step out of the way and let you iron it all out.

[ChristopherChudzicki]

@gwhitney Thank you for taking the time to review this. I think I've responded to all the comments/questions, and have made a few updates. In some places (e.g., converting multiline objects to single line) I agree with you but would prefer to leave it out of this PR. Or potentially do it as a last commit in this PR, but I'd rather make as few manual changes as possible in case we end up closing this in favor of a 4th linting PR

[gwhitney]

Well, as I've said, this sort of thing is not really my forte, so my comments were meant just as my uninformed impressions/reactions, in case those were helpful to the discussion between you and Jos. So it's extremely kind of you to go through them in such detail. I will therefore limit myself to just two small further responses to this round of discussion:

1. The comment to the @ts-expect-error should explain why there's intended to be a compile-time error, something along the lines of "... ensure that round(array, array) will not type" . Then it's not clear to me there needs to be a comment about the runtime error, since the assert.throws self-documents that.
2. I totally sympathize with not wanting to make a bunch of changes in case yet a different linting PR is pursued. But when the "one true linting PR" is selected, it seems to me that that PR is actually a good place for other purely whitespace changes for layout readability, like

{
  L,
  U
}

becomes { L, U }, since they are in the same spirit and one gets over the pain of voluminous whitespace-only changes in a single go, like ripping the band-aid off all at once minimizing the total pain.

Looking forward to getting through this :)

[ChristopherChudzicki]

Then it's not clear to me there needs to be a comment about the runtime error, since the assert.throws self-documents that.

👍

it seems to me that that PR is actually a good place for other purely whitespace changes for layout readability, like

Yeah, you're probably right. And there aren't too many. It's possible I missed a few, but I believe all two-element and three-element objects are now on a single line, if they fit. My 2c would be it's probably not worth trying to enforce this on PRs, though: If linting conventions aren't enforceable by CI, they're probably a lost cause.

[gwhitney]

My 2c would be it's probably not worth trying to enforce this on PRs,

I for one totally agree. I am a believer that it's actually best to allow some latitude in layout to the judgment of the coder, because different circumstances call for different emphasis, etc. I think the recent drive to have one automated way to lay out any given section of code is a bit overkill/misguided.

[josdejong]

Thanks a lot @ChristopherChudzicki , this is looking very good 😎

I've resolved most the conversations, a few are still open, can you have another look at those?

[ChristopherChudzicki]

@josdejong I believe I've addressed all the comments. I did add --max-warnings=0 since there are none now and preventing warnings seems desirable. Easy to revert if that change is unwanted.

[josdejong]

Thanks Christopher. Only two small conversations are still open I think (unless I'm overlooking something)

[ChristopherChudzicki]

@josdejong matrix definition extracted; let me know what you want to do with that comment. IMO, it's not any more or less confusing now than it was before this PR, so I don't think it needs to change, but if someone has a suggested re-wording, including the change here could make sense to keep history simpler.

[josdejong]

@ChristopherChudzicki thanks. Did you maybe forget to push your last commit with the matrix definition extraction? I don't see it applied yet.

ok let's leave that comment as it is, I do not really have a suggestion that improves it a lot and it's not worth spending too much time on.

[ChristopherChudzicki]

@josdejong Whoops, yes I did forget to push the commits. Pushed, and slightly tweaked those comments around @gwhitney 's suggestion

[josdejong]

Thanks again Chris for all your work, merging the PR now 👍