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

Turn XML doc and Sig<->Impl mismatch warnings on by default #10457

Merged
merged 5 commits into from
Nov 17, 2020

Conversation

cartermp
Copy link
Contributor

I think these should just be on by default.

  1. Nobody actually relies on malformed XML documentation, or documentation where things like parameter names are different in XML docs than their names in source.
  2. Sig and Impl file parameter mismatches have less of a "nobody actually relies on this" factor, but like ... nobody actually relies on parameter names differing between signature and implementation files.

This is technically a breaking change for anyone who has warnings set as errors, and should be treated like any circumstance where if we introduce a warning it will break people with that setting turned on.

That said, I think that these two checks move code towards being more correct, and especially in the case of the XML doc thing can catch some bugs that they might otherwise not easily catch.

@TIHan
Copy link
Contributor

TIHan commented Nov 17, 2020

How likely is this to impact anyone from compiling their code?

@cartermp
Copy link
Contributor Author

@TIHan For the signature file mismatch one, highly unlikely. The likelihood that someone has warnings as errors, uses signature files, intentionally mismatches parameter names, and is blocked from changing things feels like it's pretty much zero.

For the XML doc one, it will likely impact more people because there are probably several libraries out there that have malformed or incorrect XML documentation. However, the likelihood that they would appreciate this warning is also quite high, because this fixes very real bugs for their consumers.

@cartermp cartermp merged commit a4f9bcf into dotnet:main Nov 17, 2020
@cartermp cartermp deleted the default-xml-and-sig-checks branch November 17, 2020 21:42
@cartermp
Copy link
Contributor Author

Merging as per convo with @TIHan

@cartermp cartermp added this to the 16.9 milestone Nov 17, 2020
@dsyme
Copy link
Contributor

dsyme commented Nov 18, 2020

If we're turning these on, then unused variable warnings (in projects, not scripts) seem a good candidate too. These warnings catch so many bugs in real world code.

@cartermp
Copy link
Contributor Author

I wouldn't mind doing that as well, we might as well just go for it and let feedback dictate if we back off.

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

Successfully merging this pull request may close these issues.

4 participants