search

ExchangeUnion / xud

Exchange Union Daemon 🔁 ⚡️
https://exchangeunion.com
GNU Affero General Public License v3.0
115 stars 49 forks source link

style(prettier): add prettier #2016

Closed erkarl closed 3 years ago

erkarl commented 3 years ago

This PR adds prettier to enforce code style.

Depends on: https://github.com/ExchangeUnion/xud/pull/2013

erkarl commented 3 years ago

@sangaman can you take a look at the current prettier configuration .prettierrc.js and suggest a code style? I'm good with whatever the code style is as long as it's consistently enforced by prettier (fixing auto-format in my editor).

The configuration options are listed here: https://prettier.io/docs/en/options.html

Once we have agreed on the code style I'll adjust the simulation test patch files accordingly. Currently, the simulation tests are expected to fail.

sangaman commented 3 years ago

@sangaman can you take a look at the current prettier configuration .prettierrc.js and suggest a code style? I'm good with whatever the code style is as long as it's consistently enforced by prettier (fixing auto-format in my editor).

The configuration options are listed here: prettier.io/docs/en/options.html

Once we have agreed on the code style I'll adjust the simulation test patch files accordingly. Currently, the simulation tests are expected to fail.

I'll go over this soon, my goal in this PR would be just to try to come up with a set of rules that are as close as possible to our existing rules, without changing or introducing any new rules that change any significant number of existing lines of code. Then going forward we can add some more as long as they're not too drastic changes from the existing style.

erkarl commented 3 years ago

Rebased with feat/upgrade-eslint (which is rebased with latest master).

kilrau commented 3 years ago

Waiting for your review @sangaman

sangaman commented 3 years ago

I pushed some suggested rules. The three I changed:

arrowParens: 'always' - We used a mix of parentheses around single parameters and no parentheses before, I think I like always using parentheses, since it's more consistent and you won't have to go and add parentheses if you add a second parameter or a type declaration. It's also the prettier default.

printWidth: 100 - Increasing from default of 80. Discussed with karl that this was fine, I could maybe even go with more like 120. Our previous hard limit was 150 lines per character but this applied to comments and lines that couldn't easily be broken into multiple lines (like a very long string) as well. 100 seems about right to me personally in terms of a sweet spot for line length before it starts getting hard to read.

trailingComma: 'all' - This is consistent with our previous style, and I like it anyway for minimizing code diffs when adding or removing lines from comma separated lists on multiple lines in the code.

If it looks good to you @erkarl you can squash commits and resolve conflicts with the custom-xud patch application.

sangaman commented 3 years ago

I pushed a commit using 120 character line length, which still looks pretty good to me, and reduces code churn significantly from +10,996 −4,250 to +6,499 −3,281. So I'd be fine with this line length as well.

erkarl commented 3 years ago

If it looks good to you @erkarl you can squash commits

We can squash the commits now when merging the PR.

and resolve conflicts with the custom-xud patch application.

Simulation tests patch is now updated. Moving forward, what do you think about including the test code directly in the xud codebase (behind TEST_SIMULATION_XYZ environment variables). That way we wouldn't have to maintain the patch separately.

erkarl commented 3 years ago

Closing in favour of https://github.com/ExchangeUnion/xud/pull/2033