[PATCH] D29221: clang-format-vsix: "format on save" feature

[Antonio Maiorano via Phabricator via cfe-commits]

amaiorano added inline comments.


================
Comment at: tools/clang-format-vs/ClangFormat/ClangFormatPackage.cs:39
         private string style = "file";
+        private bool formatOnSaveEnabled = false;
+        private string formatOnSaveFileExtensions =
----------------
hans wrote:
> Perhaps just `formatOnSave`, similar to `sortIncludes` above?
Will do.


================
Comment at: tools/clang-format-vs/ClangFormat/ClangFormatPackage.cs:46
+        {
+            // Use MemberwiseClone to copy value types
+            var clone = (OptionPageGrid)MemberwiseClone();
----------------
hans wrote:
> Ultra nit: end the sentence with a period.
Will do.


================
Comment at: tools/clang-format-vs/ClangFormat/ClangFormatPackage.cs:88
 
-        [Category("LLVM/Clang")]
+        [Category("Format Options")]
         [DisplayName("Style")]
----------------
hans wrote:
> What do you think about using "clang-format" for the category name?
So if you take a look at the screenshot I posted with the original diff, you'll see how these categories show up:
https://reviews.llvm.org/file/data/cuztal767fqmcy2k7kkv/PHID-FILE-xcoqfwj3o2tpwbabbak5/pasted_file

"LLVM/Clang" is the main option menu name, and was always there. I figure the idea is that if we write other extensions, they'd all fall under this heading. Personally I'd like for it to be "clang-format" or "Clang Format".

As part of my change, I grouped the options that were there before under "Format Options", which is what they are (arguments to clang-format), and my new options under "Format On Save".


================
Comment at: tools/clang-format-vs/ClangFormat/ClangFormatPackage.cs:178
+
+        [Category("Format On Save")]
+        [DisplayName("Enable")]
----------------
hans wrote:
> Does this mean the "FormatOnSave" is not nested under the same category as the other clang-format options?
See my answer to the comment on line 88.


https://reviews.llvm.org/D29221