search

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

Enforcing new code to be formatted with Prettier #50

Closed dylanirlbeck closed 4 years ago

dylanirlbeck commented 4 years ago

As I was working though #49, it quickly became clear there is no standard formatting for the project (or at least a formatter that ships with the repo). I think it'd be nice to enforce Prettier for any new code that comes in to the repo, since that seems to be a fairly standard formatter for JS, CSS, HTML, and JSON -- all the tech this extension currently uses!

Foruntately for us, there is a GitHub action that will do exactly this, with minimal effort required on our part: https://github.com/creyD/prettier_action. There may be other GitHub actions, but this issue is more to start a discussion.

cc @IllyaMoskvin

IllyaMoskvin commented 4 years ago

I'm down! Using Prettier sounds good. At the risk of bikeshedding, some thoughts:

It would be nice to get Prettier setup before merging #49. For reference, I'm currently looking at this commit: https://github.com/art-institute-of-chicago/browser-extension/pull/49/commits/b06806bf9ad0db4253c6baba98b3ff931c1ac26e

I'd prefer it if the first run of Prettier was done in its own commit. I don't have an external source to reference on this, but in general, I think that when a linter/formatter is first added to a project, it's important to run it by itself, and commit whatever changes it makes separately from any changes in logic. This makes for a more readable commit history, which is something we strive per our version control guidelines.

But also, this would require you to interactive-rebase #49 on the prettified codebase, edit each commit, and allow Prettifier to run locally before continuing the rebase. We'd then need to force-push the feature/date-picker branch. I'm hesitant to ask you to go through that hassle. If you don't mind, go for it! If you'd like me to help with the clean-up and don't mind me pushing to your branch, I'd be happy to do that too.

On a different note, I disagree with a few of Prettier's defaults. (But hey, it's configurable for a reason!) I'm sure most of that is due to personal preference, but also, our web team's style tends to follow style conventions that are common in the PHP world, since that's where we do most of our work. Some of Prettier's defaults clash with the code style used in other projects our organization maintains, especially our website's codebase.

I know our website is currently closed-source, so that's not very helpful. To give some specific examples, we tend to favor single quotes over double quotes for strings. We prefer to keep trailing commas and definitely do not enforce line-length limits. These are just some examples that jumped out as I looked at the changes in the commit I linked above.

I'm down with us implementing a GitHub action for Prettier. Is this something you'd be interested in taking on?

I know Prettier should also be run in the editor locally, so we should add something to that effect to the readme.