Page MenuHomePhabricator

clang-format-vsix: "format on save" feature
ClosedPublic

Authored by amaiorano on Jan 27 2017, 8:19 AM.

Details

Summary

This change adds a feature to the clang-format VS extension that optionally enables the automatic formatting of documents when saving. Since developers always need to save their files, this eases the workflow of making sure source files are properly formatted. This feature exists in other IDEs, notably in Eclipse; furthermore, there is a VS extension that provides this functionality for generic formatters, so there is definitely a precedent for this type of feature.

Although this patch works, I know it's quite big and potentially contentious, so I'd like to start a discussion about it here. I would appreciate it if you could try it out locally for a couple days to see how it feels. Personally, I've been using it and like how it eases my workflow.

Here's a screenshot of what the options grid looks like now by default:

Things to note:

  • I renamed the category "LLVM/Clang" to "Format Options" and added one to group the "Format on Save" options.
  • The "File extensions" field is needed to filter out non-source code documents that are modified in VS, but that we don't want to format with clang-format. The list of extensions includes all possible C/C++ file extensions (I hope), and the list of extensions presently supported by clang-format.
  • The "Mode" field allows you to select "All Documents" or "Current Document", which effectively scopes the formatting on save to all modified documents or current modified document respectively. I'm still on the fence on whether to even bother offering this mode, especially since "Current Document" can be weird since you could modify multiple files, then "save all" (which happens when you trigger a build), and find that only the current document gets formatted. Perhaps it's just better to only support "All Documents" and remove this field.

Other things I'd like to call out in this change:

  • When formatting on save, we ignore the FallbackStyle in the user's options and set it to "none". This is to make sure we only format files based on Style: for e.g., if Style if "file", we only format on save when a .clang-format file is found; but we don't format files in projects that have no .clang-format. I think this makes sense since "format on save" is a global option, and users would be surprised to find it formatting files in projects that don't have a .clang-format file. The description of the "Enable" field in the options dialog explains this.
  • I split out some helper functions into a VsixUtils class and file, and added new utility functions for format on save. I also added 2 new files with more related helpers.
  • I added the attribute [ProvideAutoLoad(UIContextGuids80.SolutionExists)] to make the extension load as soon as a solution is loaded, rather than have it load lazily upon the first format line/document call. This is required so that I can register for the BeforeSave callback right away, otherwise formatting on save would not work until the user first explicitly formats (via menu or keyboard shortcut).
  • I did some refactoring around OptionsPageGrid so that rather than accessing it directly in RunClangFormat(), we pass it down as args. This allows me to override user options, specifically the FallbackStyle, when formatting on save. (line 320)

Thanks, and I looked forward to your feedback!

Diff Detail

Repository
rL LLVM

Event Timeline

amaiorano created this revision.Jan 27 2017, 8:19 AM

Hello! Just a small ping to see if anyone has taken a look at this change? I fully understand that everyone's really busy, but perhaps you can recommend another reviewer? Or if you're presently giving this change a whirl, just let me know! Cheers :)

klimek added a reviewer: hans.Feb 7 2017, 2:37 AM
klimek edited edge metadata.

+hans

+1 to "format only current document but save all" not making much sense :)

tools/clang-format-vs/ClangFormat/TypeConverterUtils.cs
7 ↗(On Diff #86046)

Perhaps name this "TypeConversion" or something - I find "utils" as part of a name to not add anything, and it tends to accumulate cruft :)

tools/clang-format-vs/ClangFormat/VsixUtils.cs
13 ↗(On Diff #86046)

Same here, just call it Vsix? (context seems clear enough)

+hans

+1 to "format only current document but save all" not making much sense :)

Yes, I've been using this version for a little while now, and it's not useful to have the current document version. I'll remove it, which will simplify the code as well.

I'll wait a bit more for comments from others before uploading a new version.

Thanks!

tools/clang-format-vs/ClangFormat/TypeConverterUtils.cs
7 ↗(On Diff #86046)

Sounds great, will do that.

tools/clang-format-vs/ClangFormat/VsixUtils.cs
13 ↗(On Diff #86046)

Perfect, will also make this change.

amaiorano updated this revision to Diff 88000.Feb 10 2017, 7:37 AM
  • Renamed VsixUtils to Vsix and TypeConverterUtils to TypeConversion
  • Removed format on save mode

The change is simpler now without the "mode" (current or all documents). Now I'm wondering whether I should rename the feature to Format _Document_ On Save, and reflect this name at least in the options grid, if not the related variable names.

Hello again. Just wondering if anyone could take a look at this? I updated the patch a week ago :) Thanks!

hans edited edge metadata.Feb 21 2017, 3:00 PM

I'm not really qualified to review the implementation details, but this seems good as far as I can tell. A few comments, mostly style related.

tools/clang-format-vs/ClangFormat/ClangFormatPackage.cs
39 ↗(On Diff #88000)

Perhaps just formatOnSave, similar to sortIncludes above?

46 ↗(On Diff #88000)

Ultra nit: end the sentence with a period.

88 ↗(On Diff #88000)

What do you think about using "clang-format" for the category name?

178 ↗(On Diff #88000)

Does this mean the "FormatOnSave" is not nested under the same category as the other clang-format options?

276 ↗(On Diff #88000)

Perhaps return early if !Vsix.IsDocumentDirty(document) instead?

amaiorano added inline comments.Feb 21 2017, 9:21 PM
tools/clang-format-vs/ClangFormat/ClangFormatPackage.cs
39 ↗(On Diff #88000)

Will do.

46 ↗(On Diff #88000)

Will do.

88 ↗(On Diff #88000)

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".

178 ↗(On Diff #88000)

See my answer to the comment on line 88.

amaiorano added inline comments.Feb 21 2017, 9:23 PM
tools/clang-format-vs/ClangFormat/ClangFormatPackage.cs
276 ↗(On Diff #88000)

Yes, will do.

amaiorano updated this revision to Diff 89815.Feb 26 2017, 12:32 PM

Made changes based on @hans 's feedback.

I looked again at the categories, and I think it makes sense with my changes. Here's an updated screenshot that shows what the options menu in Visual Studio looks like with these changes with the top-level category name ("LLVM/Clang") and page name ("Clang Format") highlighted, as well as the category names for the page items ("Format Options" and "Format On Save") highlighted:

It would be nice if "Format On Save" was _below_ "Format Options", but Visual Studio always orders the page categories in alphabetical order, so this is pretty standard.

hans added a comment.Feb 27 2017, 9:09 AM

Made changes based on @hans 's feedback.

I looked again at the categories, and I think it makes sense with my changes. Here's an updated screenshot that shows what the options menu in Visual Studio looks like with these changes with the top-level category name ("LLVM/Clang") and page name ("Clang Format") highlighted, as well as the category names for the page items ("Format Options" and "Format On Save") highlighted:

It would be nice if "Format On Save" was _below_ "Format Options", but Visual Studio always orders the page categories in alphabetical order, so this is pretty standard.

The screenshot looks good.

My only nit is that I'd prefer "clang-format" instead of "ClangFormat".

Manuel: the menu options under Tools currently say "Clang Format {Selection,Document}". What do you think about using "clang-format" here instead? That is the name of the tool after all, and I think it also works nicely as a verb.

I realize the actual extension is called ClangFormat, but maybe there were reasons for that, or we have to live with it?

In D29221#687425, @hans wrote:

Made changes based on @hans 's feedback.

I looked again at the categories, and I think it makes sense with my changes. Here's an updated screenshot that shows what the options menu in Visual Studio looks like with these changes with the top-level category name ("LLVM/Clang") and page name ("Clang Format") highlighted, as well as the category names for the page items ("Format Options" and "Format On Save") highlighted:

It would be nice if "Format On Save" was _below_ "Format Options", but Visual Studio always orders the page categories in alphabetical order, so this is pretty standard.

The screenshot looks good.

My only nit is that I'd prefer "clang-format" instead of "ClangFormat".

Manuel: the menu options under Tools currently say "Clang Format {Selection,Document}". What do you think about using "clang-format" here instead? That is the name of the tool after all, and I think it also works nicely as a verb.

I realize the actual extension is called ClangFormat, but maybe there were reasons for that, or we have to live with it?

I would also like clang-format in there rather than ClangFormat. One thing to validate is whether this change would mean people would lose their changes to the defaults in the configuration panel. I'll run some tests and see if this is indeed the case. Maybe there's a way to keep the internal name the same for config purposes, but change the displayed name instead. Will get back to you.

In D29221#687425, @hans wrote:

Made changes based on @hans 's feedback.

I looked again at the categories, and I think it makes sense with my changes. Here's an updated screenshot that shows what the options menu in Visual Studio looks like with these changes with the top-level category name ("LLVM/Clang") and page name ("Clang Format") highlighted, as well as the category names for the page items ("Format Options" and "Format On Save") highlighted:

It would be nice if "Format On Save" was _below_ "Format Options", but Visual Studio always orders the page categories in alphabetical order, so this is pretty standard.

The screenshot looks good.

My only nit is that I'd prefer "clang-format" instead of "ClangFormat".

Manuel: the menu options under Tools currently say "Clang Format {Selection,Document}". What do you think about using "clang-format" here instead? That is the name of the tool after all, and I think it also works nicely as a verb.

I realize the actual extension is called ClangFormat, but maybe there were reasons for that, or we have to live with it?

I would also like clang-format in there rather than ClangFormat. One thing to validate is whether this change would mean people would lose their changes to the defaults in the configuration panel. I'll run some tests and see if this is indeed the case. Maybe there's a way to keep the internal name the same for config purposes, but change the displayed name instead. Will get back to you.

Okay, I looked into it, and the good news is that changing the name of the category from ClangFormat to clang-format doesn't reset the previously saved changes made in the options menu. I also changed the menu item names. Here's what both would look like:

Now having said that, I'm not sure if this is the best change because of a few things:

  1. The extension's name itself is ClangFormat, which was recently added to the Visual Studio market place here: https://marketplace.visualstudio.com/items?itemName=HansWennborg.ClangFormat . Consequently, the extension appears with this name in the Visual Studio extensions dialog:
    . I don't think it would be easy to change this. You cannot really take down an extension page on the marketplace once it's there; you'd have to document that it has been deprecated and provide a link to a new page. When searching for it in the Extensions dialog in VS, though, both the old and new extension would show up.
  1. Although the name of the executable is indeed "clang-format", the documentation here uses the name ClangFormat:

So I leave it up to you whether you really want this change or not. We can also decide later rather than fold it into this change.

hans added a subscriber: djasper.Feb 28 2017, 9:22 AM

My only nit is that I'd prefer "clang-format" instead of "ClangFormat".

Manuel: the menu options under Tools currently say "Clang Format {Selection,Document}". What do you think about using "clang-format" here instead? That is the name of the tool after all, and I think it also works nicely as a verb.

I realize the actual extension is called ClangFormat, but maybe there were reasons for that, or we have to live with it?

I would also like clang-format in there rather than ClangFormat. One thing to validate is whether this change would mean people would lose their changes to the defaults in the configuration panel. I'll run some tests and see if this is indeed the case. Maybe there's a way to keep the internal name the same for config purposes, but change the displayed name instead. Will get back to you.

Okay, I looked into it, and the good news is that changing the name of the category from ClangFormat to clang-format doesn't reset the previously saved changes made in the options menu. I also changed the menu item names. Here's what both would look like:

Now having said that, I'm not sure if this is the best change because of a few things:

  1. The extension's name itself is ClangFormat, which was recently added to the Visual Studio market place here: https://marketplace.visualstudio.com/items?itemName=HansWennborg.ClangFormat . Consequently, the extension appears with this name in the Visual Studio extensions dialog:
    . I don't think it would be easy to change this. You cannot really take down an extension page on the marketplace once it's there; you'd have to document that it has been deprecated and provide a link to a new page. When searching for it in the Extensions dialog in VS, though, both the old and new extension would show up.
  2. Although the name of the executable is indeed "clang-format", the documentation here uses the name ClangFormat:


    So I leave it up to you whether you really want this change or not. We can also decide later rather than fold it into this change.

Personally I think we should refer to the tool and the action of formatting as "clang-format" as much as possible. It's unfortunate we can't rename the extension, but maybe that slight inconsistency isn't the end of the world.

Manuel, Daniel: I'd like to hear your opinions here.

In D29221#688697, @hans wrote:

My only nit is that I'd prefer "clang-format" instead of "ClangFormat".

Manuel: the menu options under Tools currently say "Clang Format {Selection,Document}". What do you think about using "clang-format" here instead? That is the name of the tool after all, and I think it also works nicely as a verb.

I realize the actual extension is called ClangFormat, but maybe there were reasons for that, or we have to live with it?

I would also like clang-format in there rather than ClangFormat. One thing to validate is whether this change would mean people would lose their changes to the defaults in the configuration panel. I'll run some tests and see if this is indeed the case. Maybe there's a way to keep the internal name the same for config purposes, but change the displayed name instead. Will get back to you.

Okay, I looked into it, and the good news is that changing the name of the category from ClangFormat to clang-format doesn't reset the previously saved changes made in the options menu. I also changed the menu item names. Here's what both would look like:

Now having said that, I'm not sure if this is the best change because of a few things:

  1. The extension's name itself is ClangFormat, which was recently added to the Visual Studio market place here: https://marketplace.visualstudio.com/items?itemName=HansWennborg.ClangFormat . Consequently, the extension appears with this name in the Visual Studio extensions dialog:
    . I don't think it would be easy to change this. You cannot really take down an extension page on the marketplace once it's there; you'd have to document that it has been deprecated and provide a link to a new page. When searching for it in the Extensions dialog in VS, though, both the old and new extension would show up.
  2. Although the name of the executable is indeed "clang-format", the documentation here uses the name ClangFormat:


    So I leave it up to you whether you really want this change or not. We can also decide later rather than fold it into this change.

Personally I think we should refer to the tool and the action of formatting as "clang-format" as much as possible. It's unfortunate we can't rename the extension, but maybe that slight inconsistency isn't the end of the world.

Manuel, Daniel: I'd like to hear your opinions here.

Hey guys, any feedback on this? About 1.5 weeks ago, @hans asked @klimek and @djasper for their opinions on "clang-format" vs "ClangFormat". Thanks!

hans added a comment.Mar 30 2017, 2:35 AM

Hey guys, any feedback on this? About 1.5 weeks ago, @hans asked @klimek and @djasper for their opinions on "clang-format" vs "ClangFormat". Thanks!

Let's get this committed now and not worry about renaming things.

Do you have commit access, or would you like me to commit it for you?

In D29221#713902, @hans wrote:

Hey guys, any feedback on this? About 1.5 weeks ago, @hans asked @klimek and @djasper for their opinions on "clang-format" vs "ClangFormat". Thanks!

Let's get this committed now and not worry about renaming things.

Do you have commit access, or would you like me to commit it for you?

I've got commit access, so I'll commit this ASAP. Thanks!

This revision was automatically updated to reflect the committed changes.