More basic XML Doco for publicly visible API #229
Conversation
because the enum fields are self-explainatory.
| @@ -23,6 +23,7 @@ | |||
| using System; | |||
|
|
|||
| // ReSharper disable CSharpWarnings::CS1591 | |||
| #pragma warning disable 1591 | |||
There was a problem hiding this comment.
We disabled 1591 in the build a while back. Why do we need to have this here?
There was a problem hiding this comment.
The assumption was that you wanted to re-enable that setting, once all the warnings were cleared up. If not, then I can stop adding semi-useless doco :)
There was a problem hiding this comment.
Do'h, just saw your PR comment. Never mind :)
There was a problem hiding this comment.
On a second thought I think we could live without 1591. The comments are enforced in the guideline and are enforced through code reviews. Enforcing them at build time just makes people write comments for the sake of writing them. So I am inclined to leave that in and leave this out if you don't mind.
There was a problem hiding this comment.
Oh I don't mind at all. Sounds reasonable.
However, in other projects, we leave it as a warning in debug builds, and have a verify configuration that tries to enforce all the rules that it can. In the verify build configuration, it would be an error to have no comments on a publicly visible member or type. Does that seem more reasonable? This way, as you're preparing to create a PR, you are reminded of the rules. Might scale a little better than explaining it in PRs.
There was a problem hiding this comment.
You're absolutely right. There aren't many warnings left and it's easier enforced in the build.
I am going to add the missing comments now and enable it again.
There was a problem hiding this comment.
Thanks for the push mate. I created #230 based on this PR and discussion! Let me know what you think.
|
I made a few minor changes and pushed this up. Please review. Thanks. |
|
Looks good. |
because eventually, CS1591 should be enabled again.