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: make clang-format and eslint agree in more places #7000

Closed
wants to merge 6 commits into from

Conversation

maribethb
Copy link
Contributor

@maribethb maribethb commented Apr 20, 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

Work on #6993

Proposed Changes

  • Updates the GitHub Action clang-format to run on generators and mocha tests as well
  • Updates the local clang-format to run on the same, and not blocks (for now, see the issue for more info)
  • Updates eslint config to use 80 as the line length, and adds an exception for template literals and import/export statements (which approximates what clang-format does)
  • Updates the clang-format rules to set JavaScriptQuotes to Leave. I do not know why I had to set this, because I checked the google style config and it's supposed to be the default option. Despite that, if I did not set it here, it changed multiple instances like const codeStringThatContainsStrings = "import 'foo'" to 'import \'foo\'' which is probably undesirable (though in all of the code in core, we do just escape the single quotes, probably because of this formatting rule). This is also important because otherwise JSON blobs in tests (which are constructed manually, not using json.stringify) would be converted to single quotes and potentially break some tests.
  • Runs the formatter (and eslint auto-fixer) on all the code after these rules are applied, so changes to non-configuration files are the result of these tools, with two exceptions:
    • Added a clang-format disable to one test with a bulky multi-line array that clang-format would butcher but isn't typical of our codebase. Apologies, forgot to stick this in its own commit but it's here
    • Added an eslint-disable to one comment that was already being ignored by clang-format. (in its own commit)
  • Fixes a missing semicolon that causes compilation errors after formatting
  • Updates the CI action to NOT lint the tests/mocha/.mocharc.js file. By default this CI action lints dotfiles but local does not. To resolve this discrepancy, you have two options:
    • Specifically exclude this dotfile
    • Have the local clang-format also run on dotfiles. This is possible by passing an option to the gulp task. But the thing clang-format wants to do to this file makes no sense. My only explanation is that somehow it thinks this file is json... there's an option to not have this space in json but not until clang-format 17, which isn't compatible with the CI action we're using

Behavior Before Change

Inconsistent formatting locally vs CI

Behavior After Change

Consistent formatting locally and on CI, and more formatted files.

Reason for Changes

Test Coverage

Documentation

Additional Information

@github-actions github-actions bot added the PR: chore General chores (dependencies, typos, etc) label Apr 20, 2023
@github-actions github-actions bot added PR: chore General chores (dependencies, typos, etc) and removed PR: chore General chores (dependencies, typos, etc) labels Apr 20, 2023
@maribethb maribethb marked this pull request as ready for review April 21, 2023 00:07
@maribethb maribethb requested a review from a team as a code owner April 21, 2023 00:07
@maribethb maribethb requested a review from cpcallen April 21, 2023 00:07
@github-actions github-actions bot added PR: chore General chores (dependencies, typos, etc) and removed PR: chore General chores (dependencies, typos, etc) labels Apr 21, 2023
@github-actions github-actions bot added PR: chore General chores (dependencies, typos, etc) and removed PR: chore General chores (dependencies, typos, etc) labels Apr 21, 2023
@maribethb
Copy link
Contributor Author

Closing in favor of #7014

@maribethb maribethb closed this Apr 25, 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.

2 participants