Skip to content

Validate XML comments in test sources during builds #499

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

Merged
merged 1 commit into from
Jul 9, 2018

Conversation

sharwell
Copy link
Contributor

@sharwell sharwell commented Jul 5, 2018

  • CS1573, CS1591, and CS1712 are disabled in test code (documentation is not required)
  • Other documentation warnings are enabled (documentation, when included, must be syntactically and semantically correct)
  • Fixes cases where comments were incorrect in the current code

Related to #434

⚠️ Please do not rewrite/rebase/squash this pull request during the merge. Edit: relaxing this request for this pull request. ⚠️

Copy link
Member

@markusweimer markusweimer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

Copy link
Contributor

@TomFinley TomFinley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @sharwell

@TomFinley
Copy link
Contributor

So, I'd happily merge this @sharwell , but was curious about one thing you note:

Please do not rewrite/rebase/squash this pull request during the merge.

Why not squash? I doubt it would have any effect on your PR, since it consists of one commit anyway.

@sharwell
Copy link
Contributor Author

sharwell commented Jul 6, 2018

Why not squash? I doubt it would have any effect on your PR, since it consists of one commit anyway.

It's a strong personal preference to not have my commits rewritten. In the event the preference cannot be accommodated¹, I prefer to have the PR closed and I would stick to filing issues and code reviews. Based on the repository history it appears that the standard merge strategy is acceptable but infrequently used, which is why I went ahead and submitted the pull request but included the note with the preference.

¹ I have no hard feelings in this case; it's happened before and I'm sure it will happen again sometime. 😄

@TomFinley
Copy link
Contributor

TomFinley commented Jul 9, 2018

Ah OK @sharwell . Hmmm. Sounds like there's a story there. Anyway, let's try this. I'll tell you the lines along which I was considering editing the commit message, and you can tell me if that's OK or not. Were I to merge without edit, the title would be, "Fix failure to validate XML comments in test sources during builds", which, frankly, I'm not 100% happy with since it mentions that something was fixed, but doesn't describe what was done. (It doesn't help that, even if we were to let people guess what was done, there are two obvious ways of doing it, suppressing validation vs. actually just fixing the comments so they pass validation, and arguably the latter is the more obvious common choice -- perhaps that it's "test" sources gives someone a hint, but I'd rather not have people rely on hints.)

So, I was going to change the thing to: "suppress XML comment validation in test code". If I was feeling especially feisty I might mention in the extended description (which is currently blank) what specific errors were addressed.

Would that level of edit be all right?

@sharwell
Copy link
Contributor Author

sharwell commented Jul 9, 2018

@TomFinley What about the PR title?

Validate XML comments in test sources during builds

We spoke on the phone and I'm OK with squash merging for this PR as a matter of repo policy. I'll keep the policy in consideration when deciding how to participate in the future.

suppress XML comment validation in test code

This would not be a correct statement. Prior to this pull request, there was no validation of XML comments in test code, so this pull request strictly increases the amount of validation performed.

@TomFinley TomFinley merged commit f7a5526 into dotnet:master Jul 9, 2018
@sharwell sharwell deleted the enable-doc-comments branch July 10, 2018 02:21
eerhardt pushed a commit to eerhardt/machinelearning that referenced this pull request Jul 27, 2018
@ghost ghost locked as resolved and limited conversation to collaborators Mar 30, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants