ci: only format and lint changed files

[yanick]

@mcous I have a counter-proposal to #292 It's a small thing, but having any change to the prettier or eslint rules causing a large amount of files to be changed makes me itch. :-) I typically prefer to have the linting done only to the files changed in the PR. The resulting npm scripts are, admitedly, a tad more complex but it'll make for more contained code changes (and a smidge faster checks).

[mcous]

Interesting! For a project with three actual source files, I think this is needlessly complex and makes the wrong tradeoffs, so I pretty strongly disagree with it, but I'll defer to your judgement 😝. Changes needed in this PR before it will work, regardless:

• PR is silently crashing in CI
• Contributing docs need to be updated
• Unrelated ESLint configuration changes should probably be punted out of this PR

Pros of this approach:

• Prettier / lint configuration can change without files changing in the same PR

Cons of this approach:

• Changing the lint configuration in one PR results in subsequent PRs containing formatting changes unrelated to the PR, just because they touched a file that hadn't been updated
• If a contributor runs npm lint or npm format, they'll still lint / format every file, which will fail in the intermediate state of "lint configuration changed but not all files updated"
• Increased complexity of run scripts and CI configuration
• This particular solution does not appear cross-OS friendly

I've been through the "introduce/update prettier" thing a few times with some very large projects, and consistently, I've found that incremental formatting annoys people and makes contributing harder. It creates muddy PRs when a file hasn't been formatted yet, and it increases the burden on contributors to not accidentally format a file they don't intend to change.

Strategies that I've found to be successful:

• Rip the bandaid off and format all files as early as possible
• Always keep formatting changes in their own style: ... PR; avoid changing formatting and other stuff at the same time
• Rely on your test suite to confirm that formatting changes don't change any runtime behavior

[mcous]

Without any rules and an overrides section mentioning .json, I don't think adding this plugin does anything

[yanick]

It does! Because in this case the list of files is passed explicitly to eslint (versus being generated off the include and exclude lists).

[mcous]

Still, we're not configuring any rules. Is this just here so a JSON parser is used and ESLint doesn't choke when a JSON file comes in the diff list? Would adding JSON to the ESLint ignore achieve the same thing?

[yanick]

I could add json files to the ignore list, but if I can get eslint to check them instead, hey, why not?

[mcous]

Why were these ignore files added? None of the paths in the globs are ESLint / prettier targets

[yanick]

Same answer as above. Both prettier and eslint are fed the file lists straight out of git diff, which returns more than what their configured include lists would have.

[mcous]

This doesn't feel very Windows friendly

[yanick]

It ain't. Are you developing under Windows?

[mcous]

Occasionally! I try to keep things shell/os-agnostic in my own projects. I dislike the idea of having a barrier to contributing based on OS when I could structure things to be more compatible

[yanick]

Yeah. I try to be as friendly as possible, but something I've learn along the years is that there is a limit on how much out of shape one should bend for the remote possibility that somebody with a specific setup might potentially want to contribute. My rule of thumb is: optimize for the comfort of who are working right now on the thing, and don't actively be a jerk for the rest.

[mcous]

• POSIX recommends $(...) over backticks for shell command substitution, IIRC
• I don't know if this is Windows friendly
• Currently, if the bash script fails, the npm script will still run, causing the failure of the script to be swallowed in CI

[yanick]

For a project with three actual source files, I think this is needlessly complex and makes the wrong tradeoffs, so I pretty strongly disagree with it, but I'll defer to your judgement

Fair enough. But having a minor change in the prettier configuration results in having 17 files worth of diffs feels worse to me. If it was a one-off instance, I could even push myself to bite the bullet, but this is something that will likely happen each time prettier or eslint fancies change (either changed by us or by these tools' defaults when they'll be upgraded).

[mcous]

My TL;DR on this is that formatting-only changes are easy to review, formatting changes mixed with other changes are hard to review. The more I sit with this, the less I like it. I hope you don't merge it; it'll make my life as a contributor harder and I think it'll make your life as a reviewer, harder, too

Fair enough. But having a minor change in the prettier configuration results in having 17 files worth of diffs feels worse to me

In my experience - including maintaining my company's shared lint configs - I haven't found this to be the case. Not every formatter/lint config affects every file, and I'd much rather skim through a formatting-only PR that is green on CI than have to review a PR that incrementally formats, leaving me with the cognitive load required to pick apart formatting changes from actual behavioral changes.

this is something that will likely happen each time prettier or eslint fancies change (either changed by us or by these tools' defaults when they'll be upgraded).

This is a really good point that I completely missed because I'm not used to working in lockfile-free projects. Setting this PR aside, I think what we need to lock our prettier and eslint versions to avoid uncontrolled drift.

If we decide to intentionally change the settings or update the versions, and it produces code formatting changes, so what? Push up a PR, let CI go green, merge, done. The whole point of having prettier is so people don't have to think about formatting. Once you introduce incremental formatting, you force contributors to think about it

[yanick]

My TL;DR on this is that formatting-only changes are easy to review, formatting changes mixed with other changes are hard to review.

This is true, but the part that I don't like is that files that haven't been actively touched for years will be periodically modified and make the git history more muddy that it needs to be.

At the end of the day, it's all a question of preferences and which trade-offs one is ready to make, and I don't think I'm ready to go with carpet bombing formatting. If somebody else was in charge I would sigh and abide, but right now since I have the keys of the car... ^.^ But since you don't like it, I will refrain from merging this branch for now.

[mcous]

I appreciate that, and I think trying to keep a clean history is a noble goal. Perhaps there is a tooling-lite method (ignore files?) of achieving this goal that we can revisit after some other changes have landed.

In the meantime, I'll pare my prettier PR down to locking the versions and keeping CI formatting checks set to warn rather than error