Rework prettier setup

[mfedderly]

Since I was already here I started by upgrading husky prettier and lint-staged.

It turns out that we intentionally dialed down the prettier aggressiveness because the formatting of test fixtures looked pretty ugly with the existing prettier settings. I think that if we increase the target printWidth for prettier, we can more confidently re-enable it for all of the test fixtures.

I think that even imperfect formatting of the fixtures is better than inconsistent formatting because of disabling it.

Its probably easiest to review this commit by looking at the first one to understand the changes, and the second commit for getting a sense of how the changes impacted the formatting across the codebase.

Thoughts? If people are on board with this, I'd like to merge this after we merge as many of the outstanding PRs as possible to give them fewer merge conflicts where possible.

[mfedderly]

Run the husky setup code before the build that people ctrl+c

[mfedderly]

This is duplicate work with the * prettier below.

[mfedderly]

^ I believe this block is no longer required in husky 9

[mfedderly]

We were actually intentionally ignoring the json/geojson files in the test directories I believe for whitespace reasons. I changed prettier to target longer lines which is maybe a better compromise with readability and gets us back to being standardized.

[mfedderly]

Open to feedback on this exact number, I essentially randomly chose it. I'm hoping that's a better compromise for test fixture geojson readability, among other things. (I think the default was 80?)

[twelch]

Okay I didn't notice this change in the previous linked PR. I agree that consistent formatting is more import than readable formatting. In that I just ran into this yesterday 😄 when a test geojson output needed to be regenerated and with all the whitespace differences, I had a hard time seeing the small bit of actual change that I needed to confirm - #2665.

Is it possible to enable or enforce a different printWidth for a subset of files? I'm not against the 120, but I do think it reduces readability of the source files. Whether code review in browser, or using multiple columns in editor like I do.

@smallsaucepan does setting to 120 support the use case that led to making the change in the first place? I would suspect not, it's seems tailored to custom formatting.

[twelch]

Here's an example from this PR where the previous 80 is better than the new 120 to my eye.

Again I could go either way.

[mfedderly]

Yeah we can totally have different prettier settings in different files with overrides. If we're all on board with re-adding json formatting to the test fixtures, I'm happy to implement whatever printWidth values the group wants.

[mfedderly]

Oh you can totally see what various prettier settings do by changing the config and just running this:

pnpm prettier --write --ignore-unknown *

It might be easiest to start from master and cherry-pick d6086161f8954b90a6be090c0171261d2a2b79c3 before running the prettier command.

[twelch]

Personally, I'd just bring back the old default setting of 80 if having a different value doesn't solve a need for the fixtures

[mfedderly]

Personally, I'd just bring back the old default setting of 80 if having a different value doesn't solve a need for the fixtures

I think there's value in having some JSON format standard even if it isn't perfectly ideal when compared to hand formatting. We can also use JSON.stringify($value, null, 2) or similar to power this if we don't like how prettier works.

[mfedderly]

@twelch I reset the prettier settings, but added a custom script that formats the json and geojson files. Thoughts?

[mfedderly]

I think the round trip of JSON.parse/stringify is modifying these which is a little scary. I think the tests all still pass though.

[twelch]

Thanks for checking it out. Modifiying the output values in the formatting step doesn't sound acceptable.

I can also see how this small text matrix before was laid out nicely, and after is non-optimal. Not sure what it looks like if it gets formatted with the default 80 printWidth instead.

[mfedderly]

Everything is going to have tradeoffs. But not automatically formatting these things isn't really an option because we can't review PRs cleanly otherwise. Here's prettier with the default printWidth #2677

[mfedderly]

See #2677