Skip to content
This repository has been archived by the owner on Jul 15, 2023. It is now read-only.

Use prettier to format source files #623

Merged
merged 39 commits into from
Nov 5, 2018
Merged

Use prettier to format source files #623

merged 39 commits into from
Nov 5, 2018

Conversation

mesaugat
Copy link
Contributor

@mesaugat mesaugat commented Nov 1, 2018

  • Added prettier and tslint-config-prettier as dev dependencies
  • Added a npm script to run prettier from the CLI: npm run prettier
  • Formats .js, .ts, .tsx, .json and .md files
  • Added .prettierrc file with a basic configuration
  • Included a .prettierignore for ignoring files that don't require prettier
  • tslint-config-prettier has been added to tslint.json. It helps in the removal of conflicts between tslint rules and prettier
  • Another npm script has been added to check for conflicting rules npm run tslint:check. The following rules were detected as conflicts by tslint-config-prettier and have been removed from tslint.json
  max-line-length
  no-irregular-whitespace
  no-unnecessary-semicolons
  number-literal-format
  object-literal-key-quotes
  quotemark
  space-within-parens
  trailing-comma
  whitespace
  • tslint:check has been added to the test script
  • recommended_ruleset.js is now generated using single quotes. Also, to avoid conflicts with prettier, recommended_ruleset.js has been added to .prettierignore
  • All files have been formatted using prettier
  • Updated tests after running prettier
  • Added husky and lint-staged to run prettier as a pre-commit hook

Fixes #558

@mesaugat
Copy link
Contributor Author

mesaugat commented Nov 1, 2018

I forgot to run the tests, it's failing. 😞

@IllusionMH
Copy link
Contributor

IllusionMH commented Nov 2, 2018

Fitrst of all - thanks for efforts and PR, unified formatting is great.

This project used single quotes that were enforced with TSLint and as I can see - there are many files with only quotes changes.
IMHO it makes sense to add singleQuote: true to .prettierrc file.

Other things to consider:

  • add prettier to the end of test script.
    There are several files that are created during npm test that will have conflicting formatting (rule-metadata.json, recommended_ruleset.js) and would be great to have them formatted accordingly.
    Do you know how long npm run prettier takes?

  • Update build-tasks/templates/rule.template.js to match formatting or run prettier after it (I can help with passing arguments to scripts within npm-run-all).

  • Update build tasks to output JSON with 4 spaces instead of 2 (not necessary if prettier script will be added to test and create-rule but nice to have)

@mesaugat
Copy link
Contributor Author

mesaugat commented Nov 2, 2018

I'll fix the tests too.

Copy link

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Awesome, this'll make our code a lot cleaner! A few small comments on top of IllusionMH's, then this should be good to merge!

package.json Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
tslint.json Show resolved Hide resolved
@JoshuaKGoldberg JoshuaKGoldberg added the PR: Waiting for Author Changes have been requested that the pull request author should address. label Nov 3, 2018
@JoshuaKGoldberg
Copy link

Yes please, a pre-commit hook would be best. Husky is pretty great.

Copy link

@JoshuaKGoldberg JoshuaKGoldberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Waiting on a pre-commit hook and recommending the VS Code extension.

.vscode/extensions.json Outdated Show resolved Hide resolved
Copy link
Contributor

@IllusionMH IllusionMH left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Checked everything except test-data and src/test.
There are 2 comments that should be fixed.

src/utils/attributes/README.md Outdated Show resolved Hide resolved
src/utils/attributes/README.md Outdated Show resolved Hide resolved
@IllusionMH
Copy link
Contributor

Also concatenation can be removed in next lines, since now that are on one line anyway (probably in follow up PR)

build-tasks/generate-sdl-report.js:13
src/chaiVagueErrorsRule.ts:10
src/chaiVagueErrorsRule.ts:12
src/insecureRandomRule.ts:7
src/insecureRandomRule.ts:9
src/jqueryDeferredMustCompleteRule.ts:26
src/missingJsdocRule.ts:28
src/noDuplicateCaseRule.ts:30
src/reactA11yRoleSupportsAriaPropsRule.ts:43
src/reactA11yRoleSupportsAriaPropsRule.ts:12

@mesaugat
Copy link
Contributor Author

mesaugat commented Nov 4, 2018

@IllusionMH I've removed the concatenations too. These were just some aesthetic changes thus pushed it here.

@IllusionMH
Copy link
Contributor

I've removed the concatenations too.
Great!

I've checked on Windows in VS Code. Both pre-commit and "editor.formatOnSave": true, works great with Prettier.

@mesaugat Thanks! There are no more notes from my side 👍

@JoshuaKGoldberg JoshuaKGoldberg added the PR: Merge Target Branch Merge conflicts exist, but no other blockers to merging. label Nov 5, 2018
@JoshuaKGoldberg
Copy link

Ahh, merge conflicts. I'll try to get to these tonight if I have permissions for the branch.

@IllusionMH
Copy link
Contributor

👍 That was fast conflict resolving 😮

@mesaugat
Copy link
Contributor Author

mesaugat commented Nov 5, 2018

Didn't have to do anything, just git pull origin master -X theirs ¯\(ツ)

@JoshuaKGoldberg JoshuaKGoldberg added PR: Waiting for Reviewer A repository maintainer should take a look at the pull request soon! and removed PR: Waiting for Author Changes have been requested that the pull request author should address. labels Nov 5, 2018
@JoshuaKGoldberg
Copy link

Tested this out locally and it works like a charm. Thanks so much @mesaugat for sending this in and working on it, and @IllusionMH for the great reviews! 🙌

@JoshuaKGoldberg JoshuaKGoldberg merged commit 9084d54 into microsoft:master Nov 5, 2018
@mesaugat mesaugat deleted the prettier branch November 5, 2018 07:28
@mesaugat
Copy link
Contributor Author

mesaugat commented Nov 5, 2018

@IllusionMH @JoshuaKGoldberg Thank you guys! 🥇

Also, I forgot to mention but I had a hard time making changes to the tests when doing this. Somehow, the tests showed an old snapshot of expected/actual even though I changed some of the tests.

I've not looked into the test utils but maybe the test results were being cached or something. These tests used to fail even though I changed it to make them green.

@JoshuaKGoldberg JoshuaKGoldberg added this to the 6.0.0-beta0 milestone Nov 6, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
PR: Merge Target Branch Merge conflicts exist, but no other blockers to merging. PR: Waiting for Reviewer A repository maintainer should take a look at the pull request soon!
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants