infra(unicorn): prefer-spread

[Shinigami92]

⚠️ #2418 needs to be merged first ⚠️

• infra(deps): add eslint unicorn #2418

[codecov]

Codecov Report

Merging #2421 (7674d33) into next (f3de0f6) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

❗ Current head 7674d33 differs from pull request most recent head 85fbd2d. Consider uploading reports for the commit 85fbd2d to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             next    #2421      +/-   ##
==========================================
- Coverage   99.59%   99.59%   -0.01%     
==========================================
  Files        2823     2823              
  Lines      255524   255509      -15     
  Branches     1105     1105              
==========================================
- Hits       254487   254466      -21     
- Misses       1009     1015       +6     
  Partials       28       28              

Files	Coverage Δ
src/internal/merge.ts	100.00% <100.00%> (ø)
src/modules/color/index.ts	99.79% <100.00%> (ø)
src/modules/company/index.ts	100.00% <100.00%> (ø)
src/modules/finance/index.ts	100.00% <100.00%> (ø)
src/modules/helpers/index.ts	98.98% <100.00%> (ø)
src/modules/internet/index.ts	100.00% <100.00%> (ø)
src/modules/string/index.ts	100.00% <100.00%> (ø)
src/modules/system/index.ts	100.00% <100.00%> (ø)

... and 3 files with indirect coverage changes

[ST-DDT]

I'm not sure whether I like this change.
The spread operator does not always conveys the same intention. E.g. slice=clone, Array.from=set to array clone, ...

[Shinigami92]

I'm not sure whether I like this change. The spread operator does not always conveys the same intention. E.g. slice=clone, Array.from=set to array clone, ...

Uhm... the spread operator is literally for cloning?! 👀

[matthewmayer]

I'm not sure whether I like this change. The spread operator does not always conveys the same intention. E.g. slice=clone, Array.from=set to array clone, ...

I agree this change seems a bit nitpicky. Javascript is not Python where "there should be one-- and preferably only one --obvious way to do it." and the spread operator isn't always the easiest to read.

Consistency is nice, but i think it can be disheartening for new contributors when they try to write some simple code and are immediately confronted with 1000 lint warnings. These kind of fixes also add a lot of churn when git blame is run and you are trying to spot the important changes.

[Shinigami92]

Javascript is not Python where "there should be one-- and preferably only one --obvious way to do it." and the spread operator isn't always the easiest to read.

Instead of trying to explain each individual diff in this PR, I just point to the documentation and you can read about why the spread is preferred over outdated patterns here: https://github.com/sindresorhus/eslint-plugin-unicorn/blob/main/docs/rules/prefer-spread.md

Consistency is nice, but i think it can be disheartening for new contributors when they try to write some simple code and are immediately confronted with 1000 lint warnings.

I always saw it the other way around: instead of getting reviewed by a human post-submitting the PR and then need to go to multiple feedback cycles, I instantly got feedback from eslint (an automatic tool that is not human-biased) and then can read the docs and even learn something about why something I wrote can be done in a better way.

These kind of fixes also add a lot of churn when git blame is run and you are trying to spot the important changes.

If you really really want to, I can introduce https://git-scm.com/docs/git-blame#Documentation/git-blame.txt---ignore-revs-fileltfilegt
But to be honest, who really cares...

[matthewmayer]

instead of getting reviewed by a human post-submitting the PR and then need to go to multiple feedback cycles, I instantly got feedback from eslint

I don't really see much evidence of that happening though. Where PR reviews have multiple rounds of reviews it's normally about something like missing tests, or disagreement about parameter naming, which linting isn't going to affect.

[matthewmayer]

Moreover these kind of frequent small changes often cause all pending PRs to need rebasing, further slowing down the acceptance of other PRs as well have to wait for the authors to fix the conflicts.

[matthewmayer]

i think my overall point is that although linting = good it doesn't necessarily mean that more linting = better. I think there comes a point where the time spent

• configuring more lint rules
• making sure they aren't controversial,
• making sure they interact correctly with other tools like prettier
• rebasing unrelated PRs

Ends up outweighing the actual time and headache saving.

Is this the correct place to draw the line? Perhaps not, but i think there is a line to be drawn at some point where we say "OK we have enough linting rules now and anything else is just personal preference/unimportant".

[ST-DDT]

Implementation wise this looks good to me.
But IMO we should talk about this in the next team meeting.

[ST-DDT]

Team Decision

• We will enable this rule for consistency reasons.