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

Address StyleCop warnings #50

Closed
adamralph opened this issue Feb 13, 2013 · 3 comments · Fixed by #62
Closed

Address StyleCop warnings #50

adamralph opened this issue Feb 13, 2013 · 3 comments · Fixed by #62

Comments

@adamralph
Copy link
Contributor

Either by fixing issues, or changing the analysis settings in each case. Preferably the former, we should try and stick to the default StyleCop settings as much as possible so that we adopt a coding style which is more compatible with other projects.

Once this is done, we should add <StyleCopTreatErrorsAsWarnings>false</StyleCopTreatErrorsAsWarnings> to the properties in the csproj files so that the build fails on violations.

@ghost ghost assigned adamralph Feb 22, 2013
@adamralph
Copy link
Contributor Author

@FakeItEasy/owners so far I've managed to address all the warnings in the distributable (net40, net35, sl) projects apart from one rule:-

SA1623 : CSharp.Documentation : The property's documentation summary text must begin with: Gets

The problem is that this language doesn't really fit for fluent API properties. E.g.

    /// <summary>
    /// The call must have happened exactly the number of times
    //// that is specified in the next step.
    /// </summary>
    public static IRepeatSpecification Exactly
    {
        get { return new RepeatSpecification((actual, expected) =>
            actual == expected, "exactly"); }
    }

I don't think it would make sense to document this with something like "Gets a repeat specification that blah blah... ".

So, we have two options:-

  1. Switch off the rule globally.
  2. Add specific suppressions.

The advantage of the former is that it's quicker and it avoids littering the code with suppression attributes but the disadvantage is that there are many places in the code where this rule should be observed and those sites will no longer be checked.

The advantage of the latter is that the rule still stays in place for those code sites which require it but the disadvantage is that we have to add a suppression attribute to each fluent API property (there are currently 72 of them), e.g.

[SuppressMessage("StyleCop.CSharp.DocumentationRules",
    "SA1623:PropertySummaryDocumentationMustMatchAccessors", Justification = "Fluent API.")]

IMHO the latter is the more 'correct', if more verbose, solution and I don't mind taking the time to add the suppressions (it shouldn't take too long) so from that point of view I should probably just go ahead. However, It would also mean that we would have to add a suppression to any new fluent API properties when implementing new features so I guess we all need to be comfortable with that.

Thoughts?

@philippdolder
Copy link
Member

@adamralph
According to http://stylecop.codeplex.com/wikipage?title=Rule%20Suppressions&referringTitle=Documentation (Suppress Message Usage) it should be possible to add the SuppressMessage attribute on the Repeated class. So we only need to add it for new classes, but not new members on existing classes.

I agree that we shouldn't suppress it globally

@adamralph
Copy link
Contributor Author

@philippdolder great! I guess in most cases the properties for a given type are either all fluent API properties or not at all so that should fit. If there is a case where it doesn't fit then I can still suppress at the member level.

In that case, I'll go ahead with suppressions. Thanks for the input.

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

Successfully merging a pull request may close this issue.

2 participants