art-institute-of-chicago / browser-extension

Browser extension to view a random artwork from our collection in a new browser tab
GNU Affero General Public License v3.0
14 stars 6 forks source link

Use Prettier for formatting #53

Closed dylanirlbeck closed 4 years ago

dylanirlbeck commented 4 years ago

Fixes #50

This PR formats the entire project using Prettier and adds a lightweight pre-commit check using pretty-quick.

@IllyaMoskvin What should I specify an option for max line length? https://prettier.io/docs/en/options.html#print-width

TODO:

IllyaMoskvin commented 4 years ago

Looking good! Thanks for putting this together. This is the first time that I've used Yarn and Prettier, so I'm documenting my onboarding process here for reference. Let me know how this differs from your workflow:

  1. Install yarn: https://classic.yarnpkg.com/en/docs/install/
  2. In the repo, run yarn install --frozen-lockfile (equivalent to npm ci)

This installs Prettier as a dev dependency, not globally. This ensures that we are all running the same version of Prettier. Different versions of Prettier could have different defaults (e.g. trailing commas).

Because we installed Prettier as a dev dependency, we'll need to run it through yarn. For example:

yarn prettier --write "$(pwd)/**/*.(js|css|html)

This will run prettier on all JS/CSS/HTML files in the project. More options described here:

https://prettier.io/docs/en/cli.html

You can also install an editor plugin. I'm experimenting with JsPrettier for Sublime Text.

For the purposes of this PR, something like this might be a decent workflow:

# Checkout the commit right before this PR
git checkout bc41582 -- "*.js" "*.css" "*.html"

# Make some modifications to .prettierrc.json
subl .prettierrc.json

# Rerun Prettier
yarn prettier --write "$(pwd)/**/*.(js|css|html)"

I'm assuming that some of Prettier's operations might be at least partially destructive (e.g. line length), so it's probably good to rerun it on the original files at every step. This won't be an issue after this PR is merged.

Also, I'm just using JS/CSS/HTML here as an example. I'm fine with using Prettier for other filetypes too.

I went ahead and pushed a couple commits to your branch, just to get a feel for it. I made Prettier ignore reset.css, since it's a standard vendor file that's included in many other projects. I set printWidth to 120 and manually fixed up a few problem areas. In one place, I added a work-around for this issue: https://github.com/prettier/prettier-vscode/issues/352

I haven't experimented with how pretty-quick works yet. I'll try to look into it this weekend!

Everything else is looking good in terms of configuration. I'll take another look before merging, but there isn't much else to change. I'd set tabWidth to 4 instead of the default 2, but it doesn't seem to be struggling with that yet. I wonder if it's just counting each indentation level twice right now.

dylanirlbeck commented 4 years ago

@IllyaMoskvin All your feedback makes sense, and the workflow you mentioned is virtually the same as mine -- I used yarn install, not yarn install --frozen-lockfile, but it appears the latter should be preferred in most cases. Moreover, I'm a Vim user, and there are plenty of extensions for using Prettier automatically.

I'd recommend taking a look at pretty-quick, as the tool should absolve you from ever needing to manually run Prettier yourself.

In terms of this PR, can you tell me what else you'd need before merging I plan on adding a section to the README about this project's use of Prettier (and some installation instructions), but after that I think everything should be ready to merge.

IllyaMoskvin commented 4 years ago

@dylanirlbeck I think that's about it. Once the README is ready, I think we'll be good to merge.

FYI I'm planning to squash this work into one commit during the merge. Normally, we'd leave the commit history intact, but as mentioned in #50, I think it's best to limit the impact of linters/formatters on git history. Makes it easier to git blame for context, etc.

I haven't looked into pretty-quick yet, but I'll do so before merging.

As an aside, I figured out why Prettier uses a tabWidth of 4 despite the default being 2. It seems that Prettier checks if there is an .editorconfig file, and uses the settings from that if none are specified in its own config. We have an .editorconfig in most of our projects, including this one. Prettier's docs don't say outright that using .editorconfig is the default, but the --no-editorconfig option implies that's the case at least for its CLI mode. So I don't think we need to add "tabWidth": 4 to .prettierrc.json.

dylanirlbeck commented 4 years ago

@IllyaMoskvin Alright this should be ready for review! I'm unable to add you explicitly as a reviewer; can you look at the repo settings to see if that option might be disabled? Thanks!

IllyaMoskvin commented 4 years ago

Merged! Thank you for the contribution! I forgot to address your request regarding reviewers. Looking at the docs, it seems that the ability to assign reviewers is reserved for people with write access to the repository. We typically reserve these rights for Art Institute employees and our contracted partners. I think for now, you can just @ me, and I'll self-assign the review.