Skip to content

Use "lint-staged" to run clang-format as pre-commit hook #3655

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.

Sign up for GitHub

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

Jump to bottom
Merged
1 commit merged into from
Nov 15, 2019
Merged

Use "lint-staged" to run clang-format as pre-commit hook #3655

1 commit merged into from
Nov 15, 2019

Conversation

@NickGerleman
Copy link
Contributor

@NickGerleman NickGerleman commented Nov 13, 2019 •
edited by ghost
Loading

Rules are based on formatFiles.js script. A directory exclusion was part of the script where the directory doesn't seem to exist, so this was removed. Tested we format .cpp and .h files in root and vnext directory, and verified we do not format files ending with .g.h or .g.cpp.

Microsoft Reviewers: Open in CodeFlow

@NickGerleman
Use "lint-staged" to run clang-format as pre-commit hook
4505c17 
Rules are based on formatFiles.js script. A directory exclusion was part of the script where the directory doesn't seem to exist, so this was removed. Tested we format .cpp and .h files in root and vnext directory, and verified we do not format files ending with .g.h or .g.cpp.
@NickGerleman NickGerleman marked this pull request as ready for review November 13, 2019 05:25
@NickGerleman NickGerleman requested a review from a team as a code owner November 13, 2019 05:25
@NickGerleman NickGerleman added the AutoMerge Causes a PR to be automatically merged once all requirements are passed (label drives bot activity) label Nov 13, 2019
@ghost
Copy link

ghost commented Nov 13, 2019

Hello @NickGerleman!

Because this pull request has the AutoMerge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

@NickGerleman NickGerleman requested review from a team and tudorms November 15, 2019 13:40
NikoAri
NikoAri reviewed Nov 15, 2019
View reviewed changes
packages/scripts/formatFiles.js
const includeEndsWith = ['.h', '.cpp'];
const excludePathContains = ['cppwinrt/winrt'];
const excludePathContains = [];
const excludePathEndsWith = ['.g.h', '.g.cpp'];
Copy link

@NikoAri NikoAri Nov 15, 2019

Choose a reason for hiding this comment

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

.g.h [](start = 30, length = 4)

Shouldn't these be already formatted properly? Why are they excluded? Our own tools are generating these files, so we have full control and they should follow proper format, thus they shouldn't be excluded.

Copy link
Contributor Author

@NickGerleman NickGerleman Nov 15, 2019

Choose a reason for hiding this comment

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

I'm not sure of the original motivation, but I could imagine it being something like what we do internally for automatic mock generation. E.g. something generates a header during the build, and will fail the build if the generated header doesn't look like the current one on disk. We could try to teach all of our tools to run clang-format like you mention, but I could totally expect their to be some external tools generating things. Thinking about things like editing an IDL file for instance.

Copy link

@NikoAri NikoAri Nov 15, 2019

Choose a reason for hiding this comment

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

For externally generated files yes, but these are not external, so they should adhere to whatever C++ formatting we define.

NikoAri
NikoAri approved these changes Nov 15, 2019
View reviewed changes
Copy link

@NikoAri NikoAri left a comment

Choose a reason for hiding this comment

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

:shipit:

@ghost ghost merged commit f516ecf into microsoft:master Nov 15, 2019
@NickGerleman NickGerleman deleted the test-lint branch November 15, 2019 18:16
hansenyy added a commit to hansenyy/react-native-windows that referenced this pull request Nov 18, 2019
@hansenyy
Squash and merge branch master.
9191338 
Performed minor clean up in ReactUWP.vcxproj.filters during merge.

Squashed commits include:

commit e7de6ce
Author: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>
Date:   Mon Nov 18 07:49:50 2019 -0800

    Bump lint-staged from 9.4.2 to 9.4.3 (microsoft#3669)

    Bumps [lint-staged](https://github.com/okonet/lint-staged) from 9.4.2 to 9.4.3.
    - [Release notes](https://github.com/okonet/lint-staged/releases)
    - [Commits](lint-staged/lint-staged@v9.4.2...v9.4.3)

    Signed-off-by: dependabot-preview[bot] <support@dependabot.com>

commit 3afbc7a
Author: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>
Date:   Mon Nov 18 07:48:52 2019 -0800

    Bump husky from 3.0.9 to 3.1.0 (microsoft#3668)

    Bumps [husky](https://github.com/typicode/husky) from 3.0.9 to 3.1.0.
    - [Release notes](https://github.com/typicode/husky/releases)
    - [Changelog](https://github.com/typicode/husky/blob/master/CHANGELOG.md)
    - [Commits](typicode/husky@v3.0.9...v3.1.0)

    Signed-off-by: dependabot-preview[bot] <support@dependabot.com>

commit 935f024
Author: React-Native-Windows Bot <53619745+rnbot@users.noreply.github.com>
Date:   Fri Nov 15 18:17:47 2019 +0000

    applying package updates ***NO_CI***

commit f516ecf
Author: Nick Gerleman <ngerlem@microsoft.com>
Date:   Fri Nov 15 10:13:34 2019 -0800

    Use "lint-staged" to run clang-format as pre-commit hook (microsoft#3655)

    Rules are based on formatFiles.js script. A directory exclusion was part of the script where the directory doesn't seem to exist, so this was removed. Tested we format .cpp and .h files in root and vnext directory, and verified we do not format files ending with .g.h or .g.cpp.

commit 6efae94
Author: Zihan Chen (MSFT) <53799235+ZihanChen-MSFT@users.noreply.github.com>
Date:   Thu Nov 14 12:11:56 2019 -0800

    Add TurboModule to ReactCommon (microsoft#3656)

    * Add jscallinvoker\jsireact and turbomodule\core to ReactCommon.vcxproj

    * Use $(ReactNativeDir)

    * Update react-native-uwp.x86.def

    * Update react-native-uwp.x64.def

    * Update react-native-uwp.arm.def

    * Update dependency to react-native

    * Use Microsoft.Windows.CppWinRT 2.0 (microsoft#3596)

    * use Microsoft.Windows.CppWinRT
    remove CreateViewManagersTests

    * Change files

    * remove unused code

    * fix format

    * pipeline run

    * delete folders to reduce disk usage

    * correct display name

    * applying package updates ***NO_CI***

    * Add support for scrollView.refreshControl (microsoft#3597)

    * Support refreshControl

    * applying package updates ***NO_CI***

    * ViewManager Command Updates (microsoft#3614)

    * Added command to CustomUserControlViewManagerCPP, microsoft#3600
    * Added support for simpler C# view manager commands signatures
    * Added support for object properties for view managers, microsoft#3613
    * Added documentation for C++ view manager
    * Added documentation for ViewManager commands, microsoft#3599

    * applying package updates ***NO_CI***

    * Use node10 (microsoft#3627)

    * Update windows-vs-pr.yml for Azure Pipelines

    * update verify job and E2Etest job

    * Native Module Setup Guide (microsoft#3628)

    * Native Module Setup Guide
    * Added NativeModuleSetup.md, microsoft#3623
    * Added link to new guide in NativeModules.md, ViewManagers.md
    * Updated ProjectStructure.md with Microsoft.ReactNative projects
    * Removed PropertySheets in SampleApps and CLI solutions

    * Change files

    * Apply suggestions from code review

    Co-Authored-By: Chris Glein <26607885+chrisglein@users.noreply.github.com>

    * applying package updates ***NO_CI***

    * Skips CI build for gif/png/jpeg (microsoft#3427) (microsoft#3632)

    * Bump @types/node from 10.17.4 to 10.17.5 (microsoft#3635)

    Bumps [@types/node](https://github.com/DefinitelyTyped/DefinitelyTyped/tree/HEAD/types/node) from 10.17.4 to 10.17.5.
    - [Release notes](https://github.com/DefinitelyTyped/DefinitelyTyped/releases)
    - [Commits](https://github.com/DefinitelyTyped/DefinitelyTyped/commits/HEAD/types/node)

    Signed-off-by: dependabot-preview[bot] <support@dependabot.com>

    * Workaround for flyout centering problem (microsoft#3640)

    * Workaround for flyout centering problem

    * adding comment explaining fix

    * Change files

    * applying package updates ***NO_CI***

    * Native Module doc fixes. (microsoft#3641)

    * Fixed C++ native module example

    * Added link to new Native Module Samples, fixed setup paths

    * Change files

    * Fixed typos

    * Don't use C++/CX

    * applying package updates ***NO_CI***

    * Update to react-native@0.60.0-microsoft.20 (microsoft#3645)

    * Update to react-native@0.60.0-microsoft.20

    * Change files

    * Change files

    * applying package updates ***NO_CI***

    * Bump @microsoft/api-documenter from 7.5.6 to 7.5.8 (microsoft#3648)

    Bumps [@microsoft/api-documenter](https://github.com/microsoft/rushstack) from 7.5.6 to 7.5.8.
    - [Release notes](https://github.com/microsoft/rushstack/releases)
    - [Commits](https://github.com/microsoft/rushstack/commits)

    Signed-off-by: dependabot-preview[bot] <support@dependabot.com>

    * Bump @microsoft/api-extractor from 7.5.4 to 7.5.6 (microsoft#3647)

    Bumps [@microsoft/api-extractor](https://github.com/microsoft/rushstack) from 7.5.4 to 7.5.6.
    - [Release notes](https://github.com/microsoft/rushstack/releases)
    - [Commits](https://github.com/microsoft/rushstack/compare/@microsoft/api-extractor-model_v7.5.4...@microsoft/api-extractor_v7.5.6)

    Signed-off-by: dependabot-preview[bot] <support@dependabot.com>

    * Do not eat exceptions in DevSupportManager::GetJavaScriptFromServer (microsoft#3616)

    * Do not eat exceptions in DevSupportManager::GetJavaScriptFromServer

    This function will currently catch all exceptions and return their message as valid Javascript. This causes havock later. E.g. running RNTester tests without a server will fail, with an Executor being told to load the JS string "Connection refused". We should throw as early as possible instead of propagating the error to a later hard-to-debug point.

    * Change files

    * applying package updates ***NO_CI***

    * Add TurboModule and JsCallInvoker files to ReactCommon only when targeting to microsoft/react-native

    * Change files

commit 68967b1
Author: Canhua Li <licanhua@live.com>
Date:   Thu Nov 14 11:57:57 2019 -0800

    remove -force flag from E2E test (microsoft#3615)

    * remove -force flag

    * remove -force flag

    * nice displayname

commit a550eab
Author: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>
Date:   Thu Nov 14 06:34:57 2019 -0800

    Bump beachball from 1.14.3 to 1.15.0 (microsoft#3659)

    Bumps [beachball](https://github.com/microsoft/beachball) from 1.14.3 to 1.15.0.
    - [Release notes](https://github.com/microsoft/beachball/releases)
    - [Changelog](https://github.com/microsoft/beachball/blob/master/azure-pipelines.release.yml)
    - [Commits](microsoft/beachball@beachball_v1.14.3...beachball_v1.15.0)

    Signed-off-by: dependabot-preview[bot] <support@dependabot.com>

commit c8ad22a
Author: React-Native-Windows Bot <53619745+rnbot@users.noreply.github.com>
Date:   Thu Nov 14 01:13:19 2019 +0000

    applying package updates ***NO_CI***

commit 486ca2b
Author: Rob Meyer <robmeyer@microsoft.com>
Date:   Wed Nov 13 17:09:38 2019 -0800

    TextInput and Switcher should respect backgroundColor, borderColor, color props for all interaction states (microsoft#3652)

    * Ensure TextControl* resource brushes are synchronized with RN backgroundColor, color, and borderColor

    * extract string constants to StandardControlResourceKeyNames.h

    * update resource brushs for Switch/ToggleSwitch

commit 901fcd9
Author: React-Native-Windows Bot <53619745+rnbot@users.noreply.github.com>
Date:   Tue Nov 12 19:25:32 2019 +0000

    applying package updates ***NO_CI***

commit edddfaa
Author: Nick Gerleman <ngerlem@microsoft.com>
Date:   Tue Nov 12 11:21:55 2019 -0800

    Do not eat exceptions in DevSupportManager::GetJavaScriptFromServer (microsoft#3616)

    * Do not eat exceptions in DevSupportManager::GetJavaScriptFromServer

    This function will currently catch all exceptions and return their message as valid Javascript. This causes havock later. E.g. running RNTester tests without a server will fail, with an Executor being told to load the JS string "Connection refused". We should throw as early as possible instead of propagating the error to a later hard-to-debug point.

    * Change files
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@NikoAri NikoAri NikoAri approved these changes

@acoates-ms acoates-ms Awaiting requested review from acoates-ms

@tudorms tudorms Awaiting requested review from tudorms

Assignees

No one assigned

Labels

AutoMerge Causes a PR to be automatically merged once all requirements are passed (label drives bot activity)

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@NickGerleman @NikoAri