Calculator: clang-format spec

[janisozaur]

Is there a chance to have a .clang-format file added to the repository so that code can be automatically formatted?

[mcooley]

I think this would be awesome, but I don't think Clang will be able to parse all our source files at the moment--see #109.

[janisozaur]

It doesn't matter. Clang-format doesn't need to understand the sources to format them.

[mcooley]

Cool, I didn't realize it was flexible enough to handle C++/CX syntax, but you're right--it appears to work. cc: @danbelcher-MSFT

[MicrosoftIssueBot]

This is your friendly Microsoft Issue Bot. I've seen this issue come in and have gone to tell a human about it.

[seyfer]

Doing it in this PR https://github.com/Microsoft/calculator/pull/236

I tried to craft the closest code style to an existing code base with its rules.
But still, it will affect every file. This is the price.
And the benefit will be that with clang-format extension every developer would be able to use autoformatting on save or with a shortcut.

[HowardWolosky]

As part of this, we need to figure out a good solution for having this run within our Azure DevOps pipeline so that our builds can catch and prevent regressions, otherwise this will be a never-ending battle.

[janisozaur]

Clang comes with tools for for integration. You can use them to format the patch (no need to run on full report) and have it report back the changes required or conformance, https://github.com/llvm-mirror/clang/blob/master/tools/clang-format/git-clang-format

[HowardWolosky]

@janisozaur / @seyfer / general community - Is there someone that can sign-up to get clang-format to run with the build automatically? I can see different final outcomes:

1. Simple - if added as a pre-build step, then it just modifies the code that was just written. Ideally, people are building and running before submitting, so clang-format will have had a chance to fix the formatting of any modified files before they try to submit.
2. More advanced - If clang-format determines that it has to format some files, it produces a build break with a message instructing users on how to fix their code files.

Are there any other suggestions on how this should be handled? In short, I _want_ this change, but I also want it to be self-sustaining long-term, so it would be great to have it hooked-up to be automated _as part_ of this change.

Thanks.

[seyfer]

@HowardWolosky thank you for the descriptive answer.
Personally, I like the simple option the most.

I just resolved conflicts in the PR.
Unfortunately, I do not have much experience with Azure builds.
If this repo has Need help tag for PRs and Issues - it is a good case to use it. :)

[janisozaur]

Since #236 is locked:

https://marketplace.visualstudio.com/items?itemName=LLVMExtensions.ClangFormat

There are completely static builds of clang-format available. The one from marketplace is all you need on Windows or you can pull one from https://github.com/angular/clang-format/tree/master/bin. Note the binary gets changed from time to time and support for new options gets added. As mentioned in #212, I will try coming up with a Linux part of CI, I will try to pull in clang format in there too

[seyfer]

Since PR is locked, answering here
@danbelcher-MSFT thank you for review. I will check it and commit with conflicts resolved on weekends.

[rudyhuyn]

Since #236 is locked:

https://marketplace.visualstudio.com/items?itemName=LLVMExtensions.ClangFormat

There are completely static builds of clang-format available. The one from marketplace is all you need on Windows or you can pull one from https://github.com/angular/clang-format/tree/master/bin. Note the binary gets changed from time to time and support for new options gets added. As mentioned in #212, I will try coming up with a Linux part of CI, I will try to pull in clang format in there too

This is not necessary, Visual Studio already includes clang-format since 15.7:
https://devblogs.microsoft.com/cppblog/clangformat-support-in-visual-studio-2017-15-7-preview-1/

[seyfer]

@danbelcher-MSFT answering here to review comment

What setting do we change so that this is reverted? The previous style is the goal.

this is not really possible to keep the previous style at all.

This setting controls at which point all cases will be broken or kept in one line
ColumnLimit: 160
So even if we have the rule to break if, constructor or else, then they are all affected by columns limit settings. And if the rule - true, but the line length < ColumnLimit then no break will happen. So it is not possible to make such small adjustments for line
int32_t const& Number::Sign() const { return m_sign; }
and if we make ColumnLimit smaller - it will break all long lines, where they were originally on one line. That's why I made it not 120 characters, but 160 characters long, it orders to keep long one-liners on one line. But it makes all shorter constructions on one line too.
I will fix all the other review comments.

The goal of this PR to have standardized code style. Here personally I do not care much about how, but only about establishing some standard for all developers.

[seyfer]

@danbelcher-MSFT if I set AlwaysBreakAfterDefinitionReturnType: true then it will be

int32_t const& 
Number::Sign() const { 
   return m_sign; 
}

is it better?
The same can be achieved with AlwaysBreakAfterReturnType

[danbelcher-MSFT]

Hey all, it seems like there is community interest in getting this change checked-in. I unlocked the PR so we can discuss details there.