Skip to content

Conversation

licanhua
Copy link
Contributor

@licanhua licanhua commented Nov 30, 2021

Fix #6658

Enforce the pipeline to check the clang-format.
User has four options to format files and apply clang-format correctly:

  1. IDE integration. https://clang.llvm.org/docs/ClangFormat.html
  2. npm run format in source/nodejs
  3. powershell -ExecutionPolicy Bypass scripts\FormatSource.ps1 -ModifiedOnly $False in project root folder
  4. If it fails the pipeline, you can download the patch like below, then run git apply downloadedfile

image

clang-format 12.0.0 or above is required.
FormatSource.ps1 used clang-format which is built into visual studio by default. so please update visual studio if you see clang-format is still 10.0
npmjs installed clang-format 14.0 automatically, so you only need to make npm install is run before npm run format

For Windows user, here is the process(a derivative of microsoft/MixedReality-GraphicsTools-Unreal)

  1. After you checked out the repo, run scripts\SetupClangFormat.bat to setup the git hooks. It's an one time setup.
  2. You edit and check in the code just as before. When you run git commit, the git hooks will verify the format for staged files
  3. If there is format issue, commit will continue, but show the error and provide command to ask user to format the file
  4. User format the files via powershell -ExecutionPolicy Bypass scripts\FormatSource.ps1 -ModifiedOnly $False -Path yourchangedfolder to format them before submitting the PR
  5. Pipeline will detect the failures if there is format issue

Error example:

c:\repo\adaptivecards>PowerShell.exe -ExecutionPolicy Bypass scripts\FormatSource.ps1 -Verify $True -Path source
[clang-format] Checking formatting: C:\repo\adaptivecards\source\uwp\Renderer\dll\dll.cpp
C:\repo\adaptivecards\source\uwp\Renderer\dll\dll.cpp:11:7: error: code should be clang-formatted [-Wclang-format-violations]
STDAPI
      ^
Errors found (see output). Please make sure to resolve all issues before opening a Pull Request.
Formatting can be applied by running:
   PowerShell.exe -ExecutionPolicy Bypass C:\repo\adaptivecards\scripts\FormatSource.ps1 -ModifiedOnly $False [-Path <path to file or directory>]

c:\repo\adaptivecards>

violations on pipeline:
image

About .clang-format, refer to https://clang.llvm.org/docs/ClangFormatStyleOptions.html

Note: In this PR, pipeline check is disabled and files will be formatted in another PR.
If you want to review the difference between the formatted files and unformatted files, you can check
8d03136

Microsoft Reviewers: Open in CodeFlow

@licanhua licanhua marked this pull request as draft November 30, 2021 20:24
@licanhua licanhua marked this pull request as ready for review December 1, 2021 00:38
@ChrisGuzak
Copy link
Member

consider committing the tools separate from the re-format.

@licanhua
Copy link
Contributor Author

licanhua commented Dec 1, 2021

consider committing the tools separate from the re-format.

good call

@paulcam206
Copy link
Member

I think we shouldn't delete source/shared/cpp/AdaptiveCardsSharedModel/AdaptiveCardsSharedModelUnitTest/.clang-format (or perhaps the new rules need to be updated), as clang-format does terrible things to our unit test code:

diff --git a/source/shared/cpp/AdaptiveCardsSharedModel/AdaptiveCardsSharedModelUnitTest/TextParsingTest.cpp b/source/shared/cpp/AdaptiveCardsSharedModel/AdaptiveCardsSharedModelUnitTest/TextParsingTest.cpp
index bc7c8afc3..fd02fdeed 100644
--- a/source/shared/cpp/AdaptiveCardsSharedModel/AdaptiveCardsSharedModelUnitTest/TextParsingTest.cpp
+++ b/source/shared/cpp/AdaptiveCardsSharedModel/AdaptiveCardsSharedModelUnitTest/TextParsingTest.cpp
@@ -7,49 +7,40 @@ using namespace Microsoft::VisualStudio::CppUnitTestFramework;
 using namespace AdaptiveCards;
 using namespace std::string_literals;

-namespace AdaptiveCardsSharedModelUnitTest
-{
-    TEST_CLASS(TextParsingTests)
-    {
-    public:
-        TEST_METHOD(HtmlEncodingPositiveTest)
-        {
-            const std::string textBlockText = _GetTextBlockText("\"Foo &amp; Bar\"&nbsp;&lt;[email protected]&gt;");
-            const std::string expectedText = "\"Foo & Bar\" <[email protected]>";
-            Assert::AreEqual(expectedText, textBlockText, L"Make sure supported HTML entities are decoded");
-        }
+namespace AdaptiveCardsSharedModelUnitTest {
+TEST_CLASS(TextParsingTests){public : TEST_METHOD(HtmlEncodingPositiveTest){
+    const std::string textBlockText = _GetTextBlockText("\"Foo &amp; Bar\"&nbsp;&lt;[email protected]&gt;");
+const std::string expectedText = "\"Foo & Bar\" <[email protected]>";
+Assert::AreEqual(expectedText, textBlockText, L"Make sure supported HTML entities are decoded");
+} // namespace AdaptiveCardsSharedModelUnitTest

-        TEST_METHOD(HtmlEncodingAmpTest)
-        {
-            // we should be doing a single pass through HTML character encoding, so the resulting "&nbsp;" below
-            // shouldn't be further decoded to " "
-            const std::string textBlockText = _GetTextBlockText("&amp;nbsp;");
-            Assert::AreEqual("&nbsp;"s, textBlockText);
-        }
+TEST_METHOD(HtmlEncodingAmpTest)
+{
+    // we should be doing a single pass through HTML character encoding, so the resulting "&nbsp;" below
+    // shouldn't be further decoded to " "
+    const std::string textBlockText = _GetTextBlockText("&amp;nbsp;");
+    Assert::AreEqual("&nbsp;"s, textBlockText);
+}

-        // Test for strings that should roundtrip without modification
-        TEST_METHOD(HtmlEncodingRoundtripTests)
-        {
-            const std::vector<std::string> testStrings =
-                {
-                    "some test text",
-                    "&foo;",
-                    "&am p;"
-                };
+// Test for strings that should roundtrip without modification
+TEST_METHOD(HtmlEncodingRoundtripTests)
+{
+    const std::vector<std::string> testStrings = {"some test text", "&foo;", "&am p;"};

-            for (const auto& testString : testStrings)
-            {
-                const std::string textBlockText = _GetTextBlockText(testString);
-                Assert::AreEqual(testString, textBlockText);
-            }
-        }
+    for (const auto& testString : testStrings)
+    {
+        const std::string textBlockText = _GetTextBlockText(testString);
+        Assert::AreEqual(testString, textBlockText);
+    }
+}

-    private:
-        std::string _GetTextBlockText(const std::string& testString)
-        {
-            TextBlock textBlock;
-            textBlock.SetText(testString);
-            return textBlock.GetText();
-        }
-    };
+private:
+std::string _GetTextBlockText(const std::string& testString)
+{
+    TextBlock textBlock;
+    textBlock.SetText(testString);
+    return textBlock.GetText();
+}
+}
+;
 }

@ghost ghost removed the Needs: Author Feedback label Dec 3, 2021
@ghost ghost removed the Needs: Author Feedback label Dec 9, 2021
@ghost ghost removed the Needs: Author Feedback label Dec 11, 2021
@licanhua licanhua requested a review from paulcam206 December 11, 2021 04:15
Copy link
Contributor

@almedina-ms almedina-ms left a comment

Choose a reason for hiding this comment

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

:shipit:

Co-authored-by: almedina-ms <[email protected]>
@licanhua licanhua dismissed paulcam206’s stale review December 14, 2021 01:55

defer the decision to remove clang-format npmjs

@licanhua licanhua enabled auto-merge (squash) December 14, 2021 01:59
@licanhua licanhua merged commit 9e97494 into main Dec 14, 2021
@licanhua licanhua deleted the clang-format branch December 14, 2021 02:21
michaelfarnsworth pushed a commit to michaelfarnsworth/AdaptiveCards that referenced this pull request Nov 10, 2022
* draft

* update readme

* format project

* update pipeline

* revert change on ios

* revert change on PerfApp because it's C++/CX project and ^ is not well supported

* update local vs version and make BasedOnStyle:Microsoft

* disable pipeline check and revert change on formatted files

* remove .h, .cpp from .editorconfig

* Apply suggestions from code review

Co-authored-by: Paul Campbell <[email protected]>

* Update .clang-format

Co-authored-by: Paul Campbell <[email protected]>

* enable hooks and recover clang-format

hook

* rollback to cpp11

* format files

* Revert "format files"

This reverts commit 700de21.

* Revert "format files"

This reverts commit 700de21.

* add clang-format-launcher

fix typo

update clang.format.json

* clang-format exception

* update documents

* add npm run format to error

* update package-lock

* add pipeline to check the format and create patch file if failed

* update echo message

* move clang.format.json to package.json

* address comments

* Deleting outdated VsCode extension (microsoft#6831)

* provide crossplatform hooks

* Update scripts/hooks/clangFormatFunc

Co-authored-by: Paul Campbell <[email protected]>

* address comment

* limit error number

* update clang-format-launcher version

* Update .clang-format

Co-authored-by: almedina-ms <[email protected]>

Co-authored-by: Paul Campbell <[email protected]>
Co-authored-by: Vsevolod <[email protected]>
Co-authored-by: Jonathan Miller <[email protected]>
Co-authored-by: almedina-ms <[email protected]>
rankush pushed a commit to rankush/AdaptiveCards that referenced this pull request May 8, 2024
* draft

* update readme

* format project

* update pipeline

* revert change on ios

* revert change on PerfApp because it's C++/CX project and ^ is not well supported

* update local vs version and make BasedOnStyle:Microsoft

* disable pipeline check and revert change on formatted files

* remove .h, .cpp from .editorconfig

* Apply suggestions from code review

Co-authored-by: Paul Campbell <[email protected]>

* Update .clang-format

Co-authored-by: Paul Campbell <[email protected]>

* enable hooks and recover clang-format

hook

* rollback to cpp11

* format files

* Revert "format files"

This reverts commit 700de21.

* Revert "format files"

This reverts commit 700de21.

* add clang-format-launcher

fix typo

update clang.format.json

* clang-format exception

* update documents

* add npm run format to error

* update package-lock

* add pipeline to check the format and create patch file if failed

* update echo message

* move clang.format.json to package.json

* address comments

* Deleting outdated VsCode extension (microsoft#6831)

* provide crossplatform hooks

* Update scripts/hooks/clangFormatFunc

Co-authored-by: Paul Campbell <[email protected]>

* address comment

* limit error number

* update clang-format-launcher version

* Update .clang-format

Co-authored-by: almedina-ms <[email protected]>

Co-authored-by: Paul Campbell <[email protected]>
Co-authored-by: Vsevolod <[email protected]>
Co-authored-by: Jonathan Miller <[email protected]>
Co-authored-by: almedina-ms <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add clang-format for c++

6 participants