-
Notifications
You must be signed in to change notification settings - Fork 57
Prettier not being used consistently #816
Description
The README states that this project uses prettier to enforce our code style, but if you run yarn prettier on the develop branch there are currently >30 files that are changed.
This seems like a problem because:
- We are not getting the benefits of a consistent code style across the project
- New developers to the project who run prettier as the README directs may think that all of the changes means there is an issue with their project setup.
- Developers will either (1) avoid running
yarn prettierwhen they make changes because of the large number of unrelated style changes they will need to separate out from their intended changes or (2) will runyarn prettierand commit those changes, resulting in PRs that are very hard to review because of the unrelated style changes.
A few options come to mind for what we can do to handle this:
- Drop our usage of prettier. I am not a fan of this because I think there is a lot of value in having a consistent code format that no one has to think about. With that said, this is the easiest solution to implement and would slightly simplify the project. Also, we just added prettier, we can't give up this soon! :-)
- Add a git hook to the project that runs prettier automatically. I have worked on another project that has taken this approach and it has worked well. It does make me feel a bit weird when there is something that changes my code automatically, but I have not seen any issues with it since prettier seems to be a very solid tool.
- Have our CI reject PRs that are not 'pretty'. This does the most to make sure that all committed code is pretty, but it will also add some friction to the development process. (push change -> build failed because you forgot prettier -> run prettier -> push prettier changes -> get PR comments, and repeat).
It appears that Option 3 was the intended path when prettier was added to the project by @maxme since one of the commit messages is "Add style check command (to be used in travis)". Maybe this decision was already made and we just need to update our build job on Travis. Of course, we could do both Options 2 & 3, so that prettier is automatically run and if you somehow manage to skip the git hook then Travis will catch it.
Once we've picked an option (assuming we don't pick Option 1), we also need to do a PR that just catches the project's code style up.