Skip to content

Add Prettier#1525

Merged
koesie10 merged 3 commits intomainfrom
koesie10/prettier
Nov 17, 2022
Merged

Add Prettier#1525
koesie10 merged 3 commits intomainfrom
koesie10/prettier

Conversation

@koesie10
Copy link
Copy Markdown
Member

@koesie10 koesie10 commented Sep 20, 2022

This adds Prettier and replaces tsfmt. VSCode is set to use Prettier for formatting TypeScript/TSX files and format on save since Prettier is very fast and does not cause any noticeable delay.

I've changed some of the default configuration of Prettier so we have less changes, but there are still many changes in formatting in almost all files. It's best to review this PR commit-by-commit: the second commit just runs npm run format and does not have any manual changes.

Checklist

  • CHANGELOG.md has been updated to incorporate all user visible changes made by this pull request.
  • Issues have been created for any UI or other user-facing changes made by this pull request.
  • [Maintainers only] If this pull request makes user-facing changes that require documentation changes, open a corresponding docs pull request in the github/codeql repo and add the ready-for-doc-review label there.

@koesie10 koesie10 requested review from a team as code owners September 20, 2022 13:13
@koesie10 koesie10 marked this pull request as draft September 26, 2022 08:02
@koesie10 koesie10 removed the secexp label Sep 26, 2022
github-advanced-security[bot]
github-advanced-security AI found potential problems Oct 25, 2022
View reviewed changes
Comment thread extensions/ql-vscode/src/query-history-scrubber.ts
await fs.remove(dir);
} else {
const timestampText = await fs.readFile(timestampFile, 'utf8');
const timestampText = await fs.readFile(timestampFile, "utf8");

Check failure

Code scanning / CodeQL

Potential file system race condition

The file may have changed since it [was checked](1).
aeisenberg
aeisenberg reviewed Oct 25, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@aeisenberg aeisenberg left a comment

Choose a reason for hiding this comment

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

I think this looks good to me, though others should take a look to be sure.

Comment thread extensions/ql-vscode/package.json
@charisk
Copy link
Copy Markdown
Contributor

charisk commented Oct 26, 2022

Do we need to update any CONTRIBUTING guidelines?

@koesie10
Copy link
Copy Markdown
Member Author

koesie10 commented Oct 26, 2022

Do we need to update any CONTRIBUTING guidelines?

I don't think so. We are not currently mentioning tsfmt or ESLint at all in the CONTRIBUTING guidelines and probably assume those will be caught automatically by the pre-commit and pre-push hooks. I think we can do the same for Prettier since VSCode will also automatically format any files on save.

@koesie10 koesie10 force-pushed the koesie10/prettier branch 2 times, most recently from 7cb044c to 9a07f2f Compare November 4, 2022 12:47
@koesie10 koesie10 marked this pull request as ready for review November 16, 2022 15:31
@koesie10
Add Prettier
f41ca1a 
This adds Prettier and makes it replace tsfmt. VSCode is set to use
Prettier for formatting TypeScript/TSX files and format on save since
Prettier is very fast and does not cause any noticeable delay.
@koesie10
Run Prettier on all files
ebcdf8a 
This will change all existing files to match Prettier formatting.

The command used is `npm run format`.
@koesie10
Add .git-blame-ignore-revs file for previous commit
f515663 
See https://www.stefanjudis.com/today-i-learned/how-to-exclude-commits-from-git-blame/
@koesie10
Copy link
Copy Markdown
Member Author

koesie10 commented Nov 16, 2022

Can we ignore the "Code scanning results / CodeQL" check? It seems like the vulnerability it detects is already present in the code and this PR just moves the code, so it isn't introducing a new vulnerability.

@robertbrignull
Copy link
Copy Markdown
Contributor

robertbrignull commented Nov 16, 2022

Can we ignore the "Code scanning results / CodeQL" check? It seems like the vulnerability it detects is already present in the code and this PR just moves the code, so it isn't introducing a new vulnerability.

Yep, should be fine to just ignore it. Thankfully the check isn't required. I agree it's just code moving around and not actually new alerts.

charisk
charisk approved these changes Nov 17, 2022
View reviewed changes
Copy link
Copy Markdown
Contributor

@charisk charisk left a comment

Choose a reason for hiding this comment

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

🚢

@koesie10 koesie10 merged commit 3e7e4b8 into main Nov 17, 2022
@koesie10 koesie10 deleted the koesie10/prettier branch November 17, 2022 08:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@aeisenberg aeisenberg aeisenberg left review comments

@charisk charisk charisk approved these changes

Assignees

No one assigned

Labels

secexp

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@koesie10 @charisk @robertbrignull @aeisenberg @github-advanced-security