F/prettier

[glebec]

Assignee Tasks

• added unit tests (or none needed)
• written relevant docs (or none needed)
• referenced any relevant issues (or none exist)

---

This PR adds automatic Prettier support.

• On commit, [JS, JSX, JSON, CSS, SCSS, MD] files will be prettified, and [JS, JSX] files will be fixed via ESLint.
• Files in .prettierignore including the bundle and package-lock.json will be ignored.
• Rules will be set according to .editorconfig and .prettierrc.yml.
• Conflicting ESLint rules have been turned off via eslint-config-prettier override.
• The source code has been run through Prettier.

[queerviolet]

I'm torn.

On the one hand, I'm lukewarm on automatic code formatters (because they have no respect for poetry), blended with a concern about student confusion that there's something rewriting their source from under them.

On the other hand, I see a lot of code that makes me cry.

@tmkelly28

[glebec]

I feel similarly. If I was working with people who put care into their formatting choices I’d not bother. But I’ve gradually found myself reflexively running Prettier on every student-written module I review because until I do the structure is buried under a mess of random indentations and bracket locations.

An alternative might be to make the repo set up for prettification but stop short of automatically running it on every commit. Make it an easy opt-in choice. EDIT: but on the other hand, it's also easy to opt out of, and in general it's the students who most need this kind of watchdog that are the least likely to opt into it.

[tmkelly28]

I would much rather this be an opt-in feature. I'd rather tell students who need it to opt-in rather than annoy students who don't.

Somewhat related note, I'm not a huge fan of some of the style decisions happening with this pull request (I like to keep my curly braces tight, dislike trailing commas, and keep a space after function keyword), but we can maybe bikeshed over that stuff later.

[glebec]

Hi @tmkelly28. OK, I'm happy to adjust it to be an opt-in feature.

Re: style decisions, Prettier by design affords very little in the way of customization. The expectation is that all engineers have small differences of opinion when it comes to style choices, but that the time is better spent just writing code however each individual author wants and passively allowing a system to normalize it, rather than forcing people to actively format their code according to a linter. I also do not agree with every single thing Prettier does, but for me the benefit is in completely letting go of such considerations and focusing entirely on logic. That being said, here is this PR's .prettierrc, which lists practically all relevant possible choices for configuration (plus their corresponding defaults):

# printWidth: 80 # 80
# tabWidth: 2 # 2
# useTabs: false # false
semi: false # true
singleQuote: true # false
trailingComma: all # none | es5 | all
# bracketSpacing: true # true
# jsxBracketSameLine: false # false
# arrowParens: avoid # avoid | always

As you can see, I've turned on semi: false, singleQuote: true, and trailingComma: all (as opposed to none) – the latter because it leads to cleaner git diffs. But I'd be happy to discuss changing any or all of these settings.

I do not know whether bracketSpacing (which is designed to affect objects and arrays) also affects ES2015 import statements. I'll try it out and report back.

---

Edit: yes, it does. Here is a before / after:

import React from 'react'
-import { connect } from 'react-redux'
+import {connect} from 'react-redux'

const AuthForm = props => {
-  const { name, displayName, handleSubmit, error } = props
+  const {name, displayName, handleSubmit, error} = props

[glebec]

Updated to be opt-in. Also re-formatted using stated preference of no space in brackets and no trailing commas.

[collin]

👍

[glebec]

Lockfile sync'd.

The open questions with this PR are as follows:

• Is it worth including this to make it easy to opt into it for groups that need or want it? Pro: it would be difficult for groups to integrate Prettier into their project without this PR, as they'd have to reinvent this code themselves from scratch. Con: it adds more noise to package.json / more magic that students don't understand.
• If it is indeed merged, are the settings satisfactory? (Keep in mind, the whole reason behind Prettier having very few settings is to remove the possibility of too much bikeshedding.)

[collin]

I've also come to this conclusion in life: My own preferences be damned, I will accept any code style choices as long as there is a tool to enforce them. This includes changing the rules later, because again, if the tool enforces it I don't have to think about it.