Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: use prettier instead of clang-format #7014

Merged
merged 12 commits into from
May 10, 2023

Conversation

maribethb
Copy link
Contributor

@maribethb maribethb commented Apr 25, 2023

The basics

  • I branched from develop
  • My pull request is against develop
  • My code follows the style guide
  • I ran npm run format and npm run lint

The details

Resolves

Fixes #6993 (by using prettier instead of clang-format)

Proposed Changes

This PR best reviewed commit-by-commit:

  • Adds Prettier tool and config; used eslint-prettier-config to remove conflicting eslint rules
  • Removes clang-format tool, config, and references
  • Reformats all files in project
    • Removed all clang-format off comments, only added two prettier-disable comments. Prettier treated some lines that had to be ignored by clang a lot better and we don't have to ignore them.
  • Updates eslint config to also lint ts files outside of core/
  • Fixes newly revealed lint problems in blocks/

Behavior Before Change

Behavior After Change

No actual behavior changes.

Notable format changes:

  • Space after function keyword in anonymous functions. This makes it consistent that there is always a space after the function keyword just like every other keyword
  • If there are too many parameters to fit on one line, each one gets its own line, with closing bracket also on own line
  • Multiple destructured imports are one per line

Reason for Changes

Clang-format doesn't handle TypeScript well. We were having to disable it entirely in the blocks/ directory. Indeed, there is even an issue where we note something that clang-format mangled. Prettier handles this better and is more readable.

Test Coverage

Tests pass. Manual inspection of files. I spot-checked several hundred files for formatting problems.

Pushed a commit with bad format to make sure the github action worked and it did.

Documentation

Additional Information

@github-actions github-actions bot added PR: chore General chores (dependencies, typos, etc) and removed PR: chore General chores (dependencies, typos, etc) labels Apr 25, 2023
@maribethb maribethb marked this pull request as ready for review April 25, 2023 21:36
@maribethb maribethb requested a review from a team as a code owner April 25, 2023 21:36
Copy link
Collaborator

@rachel-fenichel rachel-fenichel left a comment

Choose a reason for hiding this comment

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

Two questions, but overall lgtm and I'm happy to get rid of our use of that random clang-format github action.

// Assign 'this' to a variable for use in the tooltip closure below.
const thisBlock = this;
this.setTooltip(function () {
this.setTooltip(() => {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Did prettier rewrite this into an arrow function, or did you do that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I did this, to resolve lint errors related to redefining this

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh good. I was not stoked about automated rewriting of functions into arrow functions.

if (context.payload.pull_request === undefined) {
throw new Error("Can't get pull_request payload. " +
'Check a request reviewer event was triggered.');
- name: Assign requested reviewer
Copy link
Collaborator

Choose a reason for hiding this comment

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

Prettier changed indentation in a bunch of yaml files. Please confirm that it's still file/the indentation is irrelevant.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, I ran two of them through a yaml validator/formatter and they pass, and the old contents are reformatted to look like the new contents

@rachel-fenichel
Copy link
Collaborator

LGTM once you fix conflicts

@maribethb maribethb force-pushed the prettier branch 2 times, most recently from 2a33086 to 370f0da Compare May 10, 2023 22:32
@maribethb
Copy link
Contributor Author

I've fixed conflicts, re-run prettier after rebasing, and removed the requirement of clang-format from the develop branch. After pressing merge I'll require the new action instead.

@maribethb maribethb merged commit 88ff901 into google:develop May 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
PR: chore General chores (dependencies, typos, etc)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rationalize the clang-format and eslint settings
3 participants