Skip to content
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.

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

[Build Server] Automated Style Checking and Feedback #121

Open
robertkety opened this issue Mar 9, 2017 · 8 comments
Open

[Build Server] Automated Style Checking and Feedback #121

robertkety opened this issue Mar 9, 2017 · 8 comments
Milestone

Comments

@robertkety
Copy link
Contributor

I'd like to determine a means to have the build server scan incoming file changes against a style policy. Furthermore, I'd like the policy to distinguish between style requirements for our repository and style guidance we'd prefer contributors to follow. The distinction between the two types of policies determines if the pull request is rejected automatically or accepted with warnings. A pull request with no style violations or warnings (and a valid build) would be considered a pass and simply await review, authorization, and merge.
The weighted style policy would allow us to control critical style behavior (e.g., class order of fields, properties, methods, indexers, etc.) while not pestering contributors with nice-to-have style behavior (e.g., white space after semi-colon).
The determination of what qualifies as style guidance versus style requirement may happen at a later time, but a good resource might be found by examining the capabilities of 'Format Document' in Visual Studio 2015 (Edit->Advanced->Format Document OR Ctrl-K, Ctrl-D).

@persn
Copy link
Contributor

persn commented Mar 11, 2017

DotNetAnalyzers/StyleCopAnalyzers has weighted differences if I understand your criteria of weighted differences correctly.

untitled

Just like ReSharper does we can easily set certain rules to "Info" so that the IDE will suggest changes, but not enforce them with warnings or errors.

@persn
Copy link
Contributor

persn commented Mar 11, 2017

We can also set rules to pass warnings and errors, and coupled with "Pass warnings as errors" as in #124 we get CI coverage for style checking.

@persn
Copy link
Contributor

persn commented Mar 11, 2017

I'm experimenting with DotNetAnalyzers/StyleCopAnalyzers in a local branch, I may have stuff to show off tonight or tomorrow.

@persn persn mentioned this issue Mar 11, 2017
12 tasks
@persn
Copy link
Contributor

persn commented Apr 16, 2017

#159 demonstrates that we now receive feedback on style checks with CI builds. Is this satisfactory for the scope of this issue?

@robertkety
Copy link
Contributor Author

Can we do a test that demonstrates the warning condition (where the pr is not rejected, but contains style changes we'd prefer contributions not have)?

@persn
Copy link
Contributor

persn commented Apr 16, 2017

We absolutely cannot, how would that even work? The reason the current test works is because we're utilizing the "warnings as errors" attributes in our C# projects. If the StyleCop rule is set to be a warning, it will result in a failed error.

@persn
Copy link
Contributor

persn commented Apr 27, 2017

It doesn't matter, I'm been trying it out now, and I've decided the "warning-as-errors" in combination with StyleCop is just way too annoying to be considered coder friendly so I need to remove it.

@persn persn added this to the v0.2.0 milestone May 2, 2017
@persn
Copy link
Contributor

persn commented May 27, 2017

@robertkety Do we really want the use case you described? Automated feedback on warnings, but not reject the Pull Request? I just don't understand what the purpose is, I can't think of a realistic situation where a warning will show on GitHub, and I won't ask the contributor to fix it. Every warning not fixed will carry over to the next PR, bothering other contributors who didn't even introduce the warning in the first place.

In my mind the perfect scenario of what we want is:
When the contributor is coding on their computer they will get Error, Warning and Info messages like usual, nothing special about it, this will give the contributor feedback in a non-hostile way and let them code without being hindered by automatic StyleCop blocks and whatever else. However on the build server any config should fail if any Errors or Warnings are found, I repeat that any warnings not fixed will carry over and annoy others.

I'm not sure exactly how to achieve this, but I think it should be possible somehow to toggle Werror(Warning on Error) so that it is only active when on a build server, it's worth looking into.
EDIT: The easiest way to achieve this is to toggle Werror for Release builds only I suppose.

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

No branches or pull requests

2 participants