Set xo to prettier compat mode + auto fixes

[janosh]

Closes #246.

Add .prettierrc.yml and apply it to all .js files with npx prettier --write **/*.js.

I tried to find the config that resulted in the minimal amount of changes. Happy to tweak it some more though. For instance, I set printWidth: 88 following the Python formatter black but JS sometimes uses different values.

[janosh]

Ah, I forgot this project started using xo which in turn uses prettier. So I guess it makes more sense to configure xo. The downside with that is VS Code doesn't support xo format on save out of the box, I think. AFAIK, you'd to install this extension as well.

[DavidAnson]

Yes, I prefer to enforce style rules with a linter. As you note, xo is already used by this project and that is a nice way to do this. What's nice about this approach is that it is automatically enforced by CI, so even someone who doesn't know to run the right tool will be informed as part of their PR. I would prefer to enable one rule at a time so the differences are easier to understand and discuss. If people want to use an editor or extension that honors these settings, more power to them - I don't, tho, and do not want to require that.

[janosh]

I agree that running a linter in CI is great but having format on save is definitely a big time saver. Imo, would be great to have both. How about we try xo's --prettier option since that should allow us to combine without conflicts?

[DavidAnson]

Thanks for updating to use xo! I'd still love to see this added in stages (infra, rule 1, rule 2, etc.) so the diff is easier to review. I also left a comment where I think auto-fix did something weird.

[janosh]

Thanks for updating to use xo! I'd still love to see this added in stages (infra, rule 1, rule 2, etc.) so the diff is easier to review.

Happy to do it. Just bear in mind that this leads to intermittent changes (e.g. from trailing commas that are inserted when adding the --prettier flag to xo and then removed again when setting trailingComma: none in .prettierrc.yml).

[DavidAnson]

I'm happy to have the intermittent changes - right now, GitHub won't even render test.js because there are too many edits. :) Assuming you're okay with that, I'll look for an update here that adds the infrastructure and enables a single rule?

[janosh]

Assuming you're okay with that, I'll look for an update here that adds the infrastructure and enables a single rule?

The latest series of commits is not what you were asking? I must have misunderstood. Could you explain what you mean by "adds the infrastructure and enables a single rule?"?

[nschonni]

It seems like the recomendation from https://github.com/xojs/xo#prettier is to just set

	trailingComma: 'es5',
	singleQuote: false,
	bracketSpacing: true,

Not sure about some of the others.

[janosh]

Happy to use that instead. Like I said, was trying to find a config that minimizes changes.

[DavidAnson]

The way I read prettier.yml now, it is configuring and enforcing 7 style rules. That leads to so many changes to test.js that GitHub won't render it. I'm proposing enabling just a single rule at first, we review that PR and get it merged, then send a new PR enabling another rule (maybe two), PR, merge, etc.. Its a bit more process, but we have more confidence in each step along the way because the number of changes is smaller.

[janosh]

The way I read prettier.yml now, it is configuring and enforcing 7 style rules

But the additional rules result in less changes. If I leave out for example

arrowParens: avoid
trailingComma: none

there would be an even larger diff. Not sure what we'd gain by splitting into multiple PRs. This way at least you have all the effects of adding Prettier in one place.

[DavidAnson]

Okay, thanks, I'll look into this when I have some time.

[janosh]

@DavidAnson What's your view on the rule set? Minimal formatting changes (requiring more rules) or minimal rule set as @nschonni suggested?

[DavidAnson]

I don't mind a strict end state (I usually enable as many linting rules as I can), but I'd like to get there incrementally as much as possible. If that means the rule set definition changes each step along the way, no concerns from me. The code itself (and tests) is what I prioritize not introducing errors into.

[janosh]

The code itself (and tests) is what I prioritize not introducing errors into.

Understandable though both Prettier and this repo are so well tested that broken functionality from auto-formatting seems unlikely.

[DavidAnson]

From what I can tell (not a Prettier user), Prettier is not configurable enough to allow an easy rule-by-rule migration. Attempting to configure it for 0 issues with the current code as-is, I don't see how to get fewer than 222 errors. The following configuration is the best I found so far:

diff --git a/package.json b/package.json
index eb76b18..fd68447 100644
--- a/package.json
+++ b/package.json
@@ -55,6 +55,7 @@
     "xo": "^0.47.0"
   },
   "xo": {
+    "prettier": true,
     "space": true,
     "rules": {
       "comma-dangle": 0,
@@ -67,6 +68,11 @@
       "unicorn/prefer-ternary": 0
     }
   },
+  "prettier": {
+    "arrowParens": "avoid",
+    "endOfLine": "auto",
+    "trailingComma": "none"
+  },
   "ava": {
     "files": [
       "test/**/*.js",

Prettier doesn't seem to have a way to disable its indent rule and it has very strong feelings about line-breaks. It insists on Replace (files.length·>·0) with files.length·>·0 for a line like if ((files.length > 0) && !options.stdin) { which I fundamentally disagree with.

I am still experimenting, but the tool is not winning me over.

[DavidAnson]

Okay, by setting printWidth to 1000, I avoided many of the wrapping issues. This commit represents the minimal diff I got to: d62993f. I'll look over it tomorrow to form an opinion. Annoyingly, printWidth will seemingly reformat lines that have been carefully formatted for readability and jam them onto a single line (see projectConfigFiles) - which seems to defeat efforts at clarity in favor of absolute consistency.

[nschonni]

Not ideal, but you can ignore specific blocks https://prettier.io/docs/en/ignore.html

[janosh]

@DavidAnson In Prettier's defense, they do call themselves "An opinionated code formatter". There are certainly cases where it produces non-ideal formatting. For me, the benefit of not having to do the formatting manually outweighs that drawback. If it's not at all to your liking, feel free to close this PR.

[janosh]

@DavidAnson package.json needs two more lines for files to remain unchanged by prettier formatting (which was most of the goal behind this PR):

  "prettier": {
    "arrowParens": "avoid",
    "endOfLine": "auto",
    "printWidth": 1000,
    "trailingComma": "none",
+   "singleQuote": true,
+   "bracketSpacing": false
  },

Also, just my 2 cents but I think those super long lines from printWidth: 1000 aren't better than what prettier produces by default.

[DavidAnson]

Ok, thanks, I'll add those! Regarding line length, I don't love 1000, either, but chose it because it gives consistent results. Otherwise, very similar lines like in test.js get treated differently based on whether they were just above or below the length threshold and the file becomes a weird mix of seemingly random formatting.