-
Notifications
You must be signed in to change notification settings - Fork 966
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
added the missing xml comments to code & enabled 1591 #230
Conversation
@@ -27,7 +27,7 @@ | |||
<ErrorReport>prompt</ErrorReport> | |||
<WarningLevel>4</WarningLevel> | |||
<DocumentationFile>bin\Debug\Humanizer.xml</DocumentationFile> | |||
<NoWarn>1591</NoWarn> | |||
<TreatWarningsAsErrors>true</TreatWarningsAsErrors> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure you want this in debug builds? It could get annoying.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we want to enforce it on the PR, then we may as well have it everywhere. Why would this be annoying?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, say you're making/changing some classes as an experiment or 'spike'. Each time you compile, try to run tests, etc, you would be forced to put comments everywhere.
I imagine people end up putting fake comments in, just to get rid of the error, so they can compile and test our their theories.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point; but then again most people code in debug mode. So if we take this out then we're back to square one with PRs missing comments and failing on that! What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've sent a PR to do what I'm suggesting here - a separate build configuration to verify everything (rather than slow down the normal 'debug' local dev building).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool. Thanks a lot.
Disabled 1591 and warnings as errors in debug & pushed. Thanks. |
Complements #229
/cc @adamchester