Switch StyleCop violations to warnings for release builds #262

Closed
adamralph opened this Issue Feb 24, 2014 · 15 comments

Comments

Projects
None yet
5 participants
Owner

adamralph commented Feb 24, 2014

This is a proposal.

Judging from my twitter stream, StyleCop is either loved or hated.

In the interest of encouraging contribution or rather avoiding discouraging contribution, I propose that we allow PR's to be merged to master whilst containing StyleCop violations. I.e. we keep StyleCop analysis but we don't fail the build when violations are present by switching the output to warnings instead of errors.

@FakeItEasy/owners will still have the opportunity to fix up any violations after a given PR is merged and we can also expand the first point in our release notes to include StyleCop violations as well as code analysis violations.

Thoughts?

Member

philippdolder commented Feb 27, 2014

My personal opinion is, that we should not give up StyleCop violations as errors. I'm a clean code purist (also teaching people to code clean in www.bbv.ch/academy courses).

Even I disagree with some StyleCop rules (also have disabled a few in my projects). But to me it doesn't matter a lot if a certain rule would be better in the way it is or in another. What's important to me is that StyleCop is a tool that helps us keep our code consistent by a tool. IMO doing such work manually is just a waste of time if there is a tool that does the job.

And to be honest, I don't like to fix up other's coding violations that could have been done properly the first time easily.

But I could live with it, if it helps us getting more contributions. So if we go for a Fist-of-Five, I'll go for a 3

Owner

blairconrad commented Feb 27, 2014

I see both points. Me, I like the errors. I wish we had them at the Day Job. But the difference there is that we're paying the contributors–they aren't doing us favours.

FIE contributors are doing us favours, and like @adamralph, I hate to dissuade them. Then again, I wonder how contributors feel. I'm always mortified when I make an error that someone else has to clean up. I would much rather have the tools in place to help me make a clean contribution. (Also, like @philippdolder, I like receiving clean contributions.) Unfortunately, I can't tell if others are the same. If only we had numbers.

Has this been a large problem in the past? I think that @adamralph is bringing this up partly due to the failure-to-build of @Dashue's recent PR #260. In this case, I don't think @Dashue was put off by the nudge about the build failure. Rather, he seemed interested in the "fancy build rules" and quickly amended his code. @Dashue, do you mind sharing your feelings about the exchange? I think it would help shed additional light on the issue.

Contributor

Dashue commented Feb 27, 2014

As a lazy bastard I would prefer you to fix up by bad code : D Joke aside, I don't mind. It does make the pull requests take more time, and I think that could make many people don't give a damn to give back. My ocd helps me get through it : )

One thing that I don't like is that on my machine i get the error message and the project it happened in, but not file or line. Having small commits, i could guess what was causing it but it would be nice if it could point out exactly what was wrong. This was when running with rake, maybe there's some way to trigger stylecop in visual studio and that would give more detailed outputs?

As for the failure of the method not being documented, in my opinion, maybe documentation should not be up to the pull requester. Even if I were very familiar with the source I would still have trouble with providing good descriptive documentation.

Maybe it's a matter of loosening the stylecop instead of disabling it? Maybe some of the errors make PR viewers spend more time giving feedback compared to accepting the PR and fixing it themselves. I know it sucks to do "others work" but if it takes less time, maybe it's a win.

I think the errors I had was undocumented method and // comments should be proceeded by empty line. We are in different timezones and if I remember correctly I made the PR during the evening, next day @adamralph came back with build failure feedback, I fixed it that evening and I think it was merged in the next day. If I or @adamralph had other commitments I guess it could have been 2 days until feedback on first PR and 3-5 days for me to update. I guess there's no rush, but during the time maybe interest falters. Guessing

I know there's a stylecop cop in the NancyFx team, but maybe in a smaller OSS smoothness of making PR's outweigh ultimate stylecop correctness?

Take what you find valuable from this comment, I respect you for supporting FIE and will abide by any decisions made. Oh and hope it was valuable and not just a rant : )

Owner

blairconrad commented Feb 27, 2014

Thanks for the feedback, @Dashue. It's interesting, and certainly food for thought.

I don't recall having the same problem with vague errors that you had. In fact, I just built using rake and it tells me exactly which line had the problem. I wonder if we have different configuration.

Your points about the extra delays are good ones, but to me these delays can happen in conversation over any aspect of the code: function, style, naming, performance, etc. So I don't feel the StyleCop warnings make things so much worse there.

Owner

blairconrad commented Feb 27, 2014

Hey, all, I was going to make a comment before I had to come to work this morning, and maybe it's just as well that I was delayed, as @Dashue's feedback kind of ties into it.

I think rather than disabling the errors and having bad-styled code merged into master and then an owner fixing at some time, we may be better off still disallowing merge of the code with errors. Then in the case where the owners want to take on the task of fixing the errors, rather than the contributors, they could do so during a manual merge. It's about the same amount of work, and master stays clean. Thoughts?

Owner

adamralph commented Feb 27, 2014

I'm not really convinced about relying on a manual merge as the process for accepting a PR in these cases. At the moment we have a good four eyes rule on all changes to master since nothing can get in without being present on a PR and I'd like to preserve that if possible.

Presuming we do indeed want to lift the burden of StyleCop compliance from contributors, I'd prefer to allow the build to pass with violations shown as warnings so owners can merge directly from the PR branch and then send another PR which fixes the violations. This PR could be raised immediately after merging the contributor PR and thus we preserve our four eyes to master rule.

@Dashue mentions Nancy, which is considerably more active than FIE with way more contributors. Although @thecodejunkie is affectionately known as the 'human stylecop', Nancy do not enforce StyleCop compliance in builds. AFAIK they just do post merge clean ups on a best effort basis.

I'm to'ing and fro'ing with my opinion on this. I'm somewhat of a purist with these things generally and I can see no reason why contributions should ultimately not comply with the coding style of a given project, but I'm also worried about being too draconian and putting off contributors. The problem with StyleCop errors is that it's an all or nothing approach. Either you fail the build for a single error or you don't. Perhaps we can find a compromise. If there are a huge number of StyleCop violations then we ask for the PR to be fixed before merging, but if there are only a handful of violations then we allow the PR in and fix up afterwards? Thus contributors have to 'generally' follow our guidelines but not worry about every single bit of whitespace, etc. which I think a lot of people will see as overkill.

Owner

blairconrad commented Feb 27, 2014

At the moment we have a good four eyes rule on all changes to master since nothing can get in without being present on a PR and I'd like to preserve that if possible.

Oh. Perhaps my lack of Git knowledge is showing here. Could there still not be four eyes on the merge?

I guess the owner would have to branch, pull in changes from the contributor's branch, add the style-fixing commits, and then start a new PR?

I think that's technically possible, but maybe awkward?

I agree with your other statement, though. If we do want to lift the StyleCop compliance burden from contributors, your proposed approach sounds reasonable. I much prefer an immediate fix to a delayed one, just before release or something.

We do pre-pull reviews and ask people to correct violations. I know projects like SignalR does the same. Every now and then I do a general clean-up of our code, and at times I grab a pull-request locally and tweak it. It's really case-by-case. We also have our MVM's that help out with reviewing code (both for flaws and code style violations)

Owner

adamralph commented Feb 28, 2014

@thecodejunkie thanks for the input. Is there any reason that you don't run StyleCop as part of the build? Even with violations set to just produce warnings instead of errors?

Because stylecop is fucking horrible ;) We're not compliant with stylecop and I think our conventions are much clearer :)

Owner

adamralph commented Feb 28, 2014

You do know that you have full control of which rules are used? Or do you not agree with a single one of them? 😉

Too much ceremony ;) But if it works for you then knock yourself out :P

Owner

blairconrad commented Feb 28, 2014

@adamralph, I find I'm softening in my "have the violations break the build" stance. Tell me, though, if we leave the violations as warnings, will they end up getting lost in the "obsolete" warnings we already spew? I'd feel much better if we either cleaned the latter up or otherwise highlighted the StyleCop warnings.

Owner

adamralph commented Mar 1, 2014

Good question. I'll experiment a little. The obsolete warnings are there because we still have tests for obsolete parts of the API (probably a good thing to avoid making breaking changes until we remove them) but we should be able to suppress the warnings in code.

@adamralph adamralph self-assigned this Mar 4, 2014

Owner

adamralph commented Nov 26, 2014

So the obsolete warnings are gone with #344, in which case I'll go ahead with this change.

@adamralph adamralph added in-progress and removed 0 - Backlog labels Nov 26, 2014

@blairconrad blairconrad closed this in #397 Nov 27, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment