Turfjs / turf

A modular geospatial engine written in JavaScript and TypeScript
https://turfjs.org/
MIT License
9.25k stars 937 forks source link

Rework prettier setup #2670

Closed mfedderly closed 1 month ago

mfedderly commented 1 month ago

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.

twelch commented 1 month ago

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 - https://github.com/Turfjs/turf/pull/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 commented 1 month ago

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

image

Again I could go either way.

mfedderly commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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 commented 1 month ago

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

mfedderly commented 1 month ago

See #2677