Run Prettier #7279
Run Prettier #7279
Conversation
Codecov Report
@@ Coverage Diff @@
## master #7279 +/- ##
==========================================
- Coverage 93.91% 93.91% -0.01%
==========================================
Files 179 179
Lines 13154 13153 -1
==========================================
- Hits 12353 12352 -1
Misses 801 801
Continue to review full report at Codecov.
|
CONTRIBUTING.md
Outdated
| @@ -114,6 +114,7 @@ Once you have babel running in watch mode, you can start making changes to parse | |||
| * Take testing seriously! Aim to increase the test coverage with every pull request. To obtain the test coverage of the project, run: `npm run coverage` | |||
| * Run the tests for the file you are working on with the following command: `npm test spec/MyFile.spec.js` | |||
| * Run the tests for the whole project to make sure the code passes all tests. This can be done by running the test command for a single file but removing the test file argument. The results can be seen at *<PROJECT_ROOT>/coverage/lcov-report/index.html*. | |||
| * Format your code by running `npm run prettier` followed by `npm run lint-fix`. | |||
There was a problem hiding this comment.
We could update the prettier command in package.json and add lint-fix to it so that this is one step instead of two.
There was a problem hiding this comment.
To chime in, we have had this discussion some time ago and I suggested to keep those two apart for the contributor's benefit to be able to run either one, because prettier can be quite opinionated. Maybe a clean script could execute both?
|
Added |
There was a problem hiding this comment.
I think running this manually every time doesn't make much sense, nor does relying on the contributor to run it, and it obfuscates the blame lines.
Lint is currently a CI check, see check-lint action. Should we replace it with check-clean that includes prettier? Also we could think about a pre-commit hook that runs prettier, although I always find it irritating when there are automatic changes before commit.
|
I agree, I think that makes sense. The problem I see with having it run with the CI, is that prettier is often different to lint. So, prettier might throw an error because the format isn't right, when it's actually perfectly fine according to lint-fix (this often happens - prettier changes a file, lint changes it back). How would you see resolving that problem? I think it makes sense to add prettier into the pre-commit hook, otherwise we're going to continually have contributions that forget to run prettier, even further obfuscating the blame lines as you've said. As far as I know too, a pre-commit hook would only run on affected files. |
|
I am pretty sure we had pre-commit hooks at some point, and I didn't like it because it always reformatted the code and made it hard to read in some cases. Prettier is quite opinionated. So I wonder if we can/should have a final clean up at the end as part of the CI "on merge" that makes a prettier commit. |
New Pull Request Checklist
Issue Description
Just like #7172, this is just runs
npm run prettierand thennpm run lint-fixon the master branch.Out of habit I run
prettierandlintprior to submitting a PR, however there are a number of files that haven't had this process ran through on them.The result is this PR.
Also adds the line to CONTRIBUTING: