Best practices for XML doc comments

[jnm2]

(Edit: I've replaced 'coding standards' with 'best practices' because I meant something very different by it than the way it is intended to be used in the context of https://github.com/nunit/docs/wiki/Coding-Standards.)

I've noticed our XML documentation is a little inconsistent. Some sentences have missing periods and some single nouns with an adjective or two do have periods. Leaving periods off of sentences is, as a starting point, neither generally a professional style nor a standard in the .NET BCL.

I'd vote for requiring the period no matter the length of the description and in scenarios where it looks awkward, rather than dropping the period, add a couple more words to make a sentence.

I think this is one of the most important coding standards best practices to have. It's user-facing and extremely visible. It reflects on the quality of NUnit as a whole. Whatever we settle on, could we put it in https://github.com/nunit/docs/wiki/Coding-Standards a coding best practices doc? I can do that and a pass over the codebase.

/cc @nunit/framework-team

---

My bucket list for other XML documentation coding standards best practices (draft):

• Consider <paramref name="parameterName"/>, <see cref="TypeOrMember"/> and sometimes even <see langword="null"/> when possible.
  This creates links in the Object Browser and intellisense and colorizes intellisense. The compiler checks the validity of the names and overloads and you can refactor with confidence.

• Add a <summary> tag for each type and member. However, prefer to give each type and member a really communicative name. If those names end up covering all the user could need to know, remove the <summary> tag to avoid super redundant documentation. Otherwise, add relevant details.

• Add a <param> tag for each parameter describing what effect it has (rather than what it is). However, prefer to give each parameter a really communicative name. If those names end up covering all the user could need to know about all the parameters, remove all the <param> tags to avoid super redundant documentation. Otherwise, add relevant details to each <param> to the extent possible.

• Don't spend any time on the <remarks> or <returns> tags since the contents are not typically seen. Important details should all be in the <summary>. If the IDE auto-inserts them, just remove them.

• Don't leave any empty tags. Either remove them or fill them out. This includes the compiler's all-or-none <param> tag enforcement.

• Consider documenting thrown exceptions with the <exception> tag.
  This can really get the consumer up to speed on things that aren't immediately obvious from the method signature.
  Sadly intellisense will only show the exception types thrown, not the message, so for important exceptions it's good to also include it in the summary:

  /// <summary>
  /// Does foo. If <see cref="OtherProperty"/> is not set, throws <see cref="InvalidOperationException"/>.
  /// </summary>
  /// <exception cref="InvalidOperationException">Thrown when <see cref="OtherProperty"/> is not set.</exception>

[ChrisMaddock]

Sounds good - all for setting up a standard. There's a fair bit of incorrect xmldoc which could also do with fixing. (Wrong params etc.)

I presume you plan to restrict all the above to just public facing types?

[jnm2]

@ChrisMaddock

I presume you plan to restrict all the above to just public facing types?

Good point. That's the motivation and primary urgency, although I do think that as we write code we should at least consider following it for internal types too on a case by case basis. It would sure help when getting up to speed reviewing PRs. Not sure what others think.

[CharliePoole]

I would vote against adding much more to our coding standards. Coding standards should be minimal and only include those things that we are willing to impose on everyone. If we are specifying periods, then I think we are getting overly detailed.

I often tell teams to think of the coding standard as if it were the boss - that is, would you like it if your boss were continually telling you to do this thing. In a self-driven team, the coding standard is, of course, not a boss. The team is the boss and nothing should be in the coding standard unless the team actually agrees with it. Don't put stuff in and say we can make an exception - just put in stuff for which you won't make any exceptions.

This all comes from a particular point of view, of course. Forty years in the industry with the last 15 as a coach, working with teams to set up XP practices, including coding standards. I never found a coding standard in the wild that couldn't be shortened. They almost always include things that nobody really cares enough about. Make the coding standard about how to write minimally acceptable code, not perfect code. IOW, if you put periods in the coding standard, then that should mean you plan to reject PRs for want of a period and that seems silly to me.

[CharliePoole]

@jnm2 You have made this apply to the framework only by virtue of putting it here. Is that your intent?

Further, regarding XML comments, they are not required on public-facing types at all unless we are talking about an assembly that requres them. Not all of our assemblies do.

[jnm2]

Yes, I think the polish on NUnit's public-facing types is important enough that I would request changes on any PR that does not include a period, as I have been doing. That's the minimal standard that I personally consider as important for professionalism as requesting that a typo be fixed.

And yes, framework only. The framework is the only place where end users see XML documentation, right?

[CharliePoole]

@jnm2 I guess I might have said first that I do like all your points if you are talking about a guide for how to write good XML comments. But you said coding standard, which is a set of mandatory requirements, and that's what I was focusing on.

[CharliePoole]

@jnm2 OK, just checking about the framework.

Here's my take on what a coding standard means...

If you want a period on the end of every sentence, you can request it whether the coding standard says it or not. That's how we do almost everything. Programmers can take your advice or not, but mostly take it. That's the level of authority I had as a Coach for fifteen years. People did what I suggested because they respected my opinion. People will do the same when you review their code, with or without a coding standard to give you authority.

OTOH, if it's in the coding standard, you or anyone else can demand that the period be added. A Coding Standard is not a bunch of suggestions or a guideline, but a set of fairly arbitrary requirements. It's particularly good for things that could go one way or another, but for which we want to standardize, like naming conventions. That's why I say that a Coding Standard should be minimal in any self-directing team. Obviously, this is totally different from any corporate standards that are handed down from above.

[jnm2]

To clarify where I'm coming from, good public XML documentation and exception messages without misinformation, poor grammar or misspellings is no less important from my perspective than good implementation without bugs or undue sloppiness. Just like I wouldn't consider it an okay state of things to have failing tests and buggy code in master, I wouldn't consider it an okay state of things to have poor XML documentation.

I generally agree with you about coding standards. It's a really good point that I like so much that I keep hoping for a conversation to come up where I can agree with you about its application! 😃 This is making me realize that XML documentation (and error messages) are unique from all other coding standards. It's almost a separate product from the implementation. I'd personally be happy to enforce an invariant to achieve the goal of professional user-facing XML documentation in exactly the same manner that we enforce invariants of testing on the implementation.

[jnm2]

But that's just my vote. I'm interested in your and the rest of the team's reaction to the idea. Do you have a vision of some kind of long-term bar to be set for XML documentation quality? How high would the bar be? Is it better to stay as we are with no long-term bar?

[jnm2]

I guess I might have said first that I do like all your points if you are talking about a guide for how to write good XML comments. But you said coding standard, which is a set of mandatory requirements, and that's what I was focusing on.

One more factor. Contributors go looking for standards to follow. I do it, I see evidence of others doing it. They usually look at the coding standard and at the nearby files to decide how to proceed. We should have some way to document recommendations to follow at the contributor's discretion even if we do not wish to make them a coding standard.

[CharliePoole]

@jnm2

"good public XML documentation and exception messages without misinformation, poor grammar or misspellings is no less important from my perspective than good implementation without bugs or undue sloppiness. Just like I wouldn't consider it an okay state of things to have failing tests and buggy code in master, I wouldn't consider it an okay state of things to have poor XML documentation."

<chiding>
I'm sorry, but I have to call you on that one. It's a fallacy to assume that anyone who disagrees with you must not share your values of good workmanship. That, in fact, is the entire point I'm trying to make about standards. If one writes a standard that says "documents must have properly spelled words" for example, one is not only stating the obvious but insulting one's fellow team members. That is exactly the sort of standard I have found in many corps, where a separate organization created policies and procedures for programmers to follow. Just as we do not write a standard that says "C# programs must be properly written and follow good principles of organization" we don't want one that says the same about XML documentation.

Our disagreement is not about documentation or it's value or about workmanship in general. It's about the nature and purpose of a coding standard.

To suggest that one who disagrees with you values good workmanship less than you do is an ad hominem argument of the worst sort.
</chiding>

Sorry 'bout that. 😄

The term "Coding Standard" (in caps) is a term of art in most agile and definitely in XP development. It does not mean any of the things I'm seeing suggested as the purpose of this coding standard. The Coding Standard we have was written, principally by me, using the XP sense of the term. You guys could change that, but since there are lots of other words, I hope you won't.

What you describe is (to my mind) more like "Best Practices in XML Documentation" whereas a Coding Standard is (in my definition) more like "Minimal Practices." It wouldn't matter except that I think we actually need something that describes a bare minimum a lot of the time.

For example, much of what is done on NUnit is done by casual contributors. Now, if we send people away when they have not followed all your best practices, then they may not come back and we will have fewer contributors. OTOH, if we accept their work and then work to improve it as well as to get them to do better next time we will have a better chance of retaining them.

Finally, I have to add this: if your ideas are so good, why do you need the force of "legal" authority behind them? 😸 Personally, I don't think you do.

[CharliePoole]

Our highest standard is that we won't merge anything unless two committers agree to it. Everything else is teaching, not enforcement.

[jnm2]

It's a fallacy to assume that anyone who disagrees with you must not share your values of good workmanship.

I'm sorry! Not the implication I intended at all. I agree with what you are saying.

Here's what I'm trying to learn. If we all end up agreeing that we want to be in a place where every XML documentation sentence has an end mark because we think that is a better consumer experience, then we have to choose how we get to that place:

• We could spot and request changes in PRs.
• We could review the codebase fix it ourselves periodically.
• We could write a test that checks it and warns or errors. (This saves the contributor's time and keeps us from missing stuff.)
• We could add it to a coding standard if we aren't willing to ship without someone fixing the style or a best practices document if we are. (This saves the contributor's time when they look for guidance. Can result in higher quality XML documentation than we'd feel comfortable holding up a PR over.)

So if I've asked more clearly, what am I overlooking?

Now, if we send people away when they have not followed all your best practices, then they may not come back and we will have fewer contributors.

I was envisioning requesting changes on PRs, but I'm not sure I'm understanding what you mean here. How does sending people away look?

[ChrisMaddock]

Perhaps there's something of a divide between how our Coding Standards are currently used, and what we want them to be. I just had a quick flick now - there's bits and pieces in there I didn't know about, and would never have enforced in review, or even written in my own code! 😄

My thoughts:

• A document to provide xmldoc format guidelines sounds good - exactly what we call that document, I'm not too worried!
• I'd also be wary of sending people back just to correct a couple of full-stops on a PR. However, other projects do operate like that (Cake) - and it seems to work well enough for them.
• I'd also hope that, once the existing xmldoc is more standardised, people will be able to pick up convention, and the above may not be such a common issue.

[jnm2]

I'd also be wary of sending people back just to correct a couple of full-stops on a PR.

If the contributor sees it in the same light as a typo, via guidelines or by seeing what nearby files do, it's possible they'd even be eager to fix it. It depends, is that how we want to see it and present it?

[oznetmaster]

I agree with @CharliePoole. Less is better!

[oznetmaster]

I actually dislike the period requirement. I think it clutters things up. I have never used periods in my xml comments. No one has ever complained on any project I have been involved with.

[jnm2]

Thanks for that feedback, @oznetmaster.

We could:

• Do nothing (I might ask for end marks on sentences... 😈)
• Decide it's okay for NUnit to have mixed styles in its XML documentation (I stop asking for such changes)
• Decide we want one or the other style

[CharliePoole]

@jnm2 Starting over! I only disagreed with two words in your original proposal: "Coding Standard." I actually agree with most everything else - I'm avoiding cluttering this discussion with the details right now.

Perhaps I should have answered you initially by saying "Thanks. I agree with most of that. I think it's better as a guidance document like "Best Practices for XML Document Comments" rather than a coding standard. Chances are we would have settled it quicker.

Recapping: My background, at least the part of my background I'm most proud of, is as an XP guy. In XP, "Coding Standard" has a specific meaning, which may not be what it means to you. It's a collection of rather unimportant stuff that we settle on once and for all so as not to spend all our time arguing. Stuff like placement of curly braces, whether to use a prefix on member fields, etc.

Most of the experts I respect say that you don't put best practices in a coding standard. You allow the team to evolve its own best practices. Many times, there are several "best" (or darned good) practices and it's not actually necessary to choose between them once and for all. It's called "Trusting people over policies and procedures."

This is just my background, and probably not yours. In the past, I got to impose my own personal beliefs on this project. Shortly, I won't get to do it any more. That's why we are trying to set up a governance model. But thinking about how we run things does not mean to me that we want to have more policies and procedures. It's more about how we resolve a question when there are multiple opinions.

But as I said, I don't think we are that far apart but are just focusing on different things. When you say Coding Standard, I hear something that must absolutely be followed - there is no judgement. I want it to be minimal because I want people to use judgement. If we have a set of best or recommended practices, most people will use their judgement by following them. In the end, those people who spend the most time in the code will "win" by making things the way they think they should be - and that's as it should be in open source.

[oznetmaster]

Usually an element of an xml comment is a single "sentence".

If not, I usually use the para element for each "sentence".

[CharliePoole]

@jnm2 Seems to me that the logical thing to do is for you to write something about what you think should be the best practices for XML documentation. I say you, because you already started in a comment above. Then we can debate the various points and maybe settle on something we will follow just as we tr to follow good principles of coding and design.

[oznetmaster]

I think that even trying to write "best practices" for xml documentation could be a slippery slope.

Immediately, it raises questions like "which English do we spell check against" ? What culture do we use for numerics and for list separators? One space or two after a period (if we allow/require periods).

I really think this is a bit far afield from where we should be focusing our attentions!

[oznetmaster]

Is there either/or a coding standard or a best practices for the documentation pages?

[jnm2]

@CharliePoole Okay, I think we're on the same page now.

@oznetmaster I would think those questions are out of scope for XML documentation, at least at the moment. And if the NUnit framework team did somehow come to a consensus that there was a need to format lists a certain way, why would that be a bad thing? It's really up to the team?

[oznetmaster]

I do not see why they are out of scope. They are as relevant as punctuation for a sentence.

Is "color" to be spelt color or colour in the documentation? Is a digits separator in a number to be a comma or a period? Same for the decimal separator. The list "a, b, c, d" is written "a; b; c; d" in Germany.

I believe that your whole premise is that the xml documentation for the public facing side of the framework needs to be consistent because it is how the NUnit users most frequently "see" NUnit. This is part of being consistent.

[jnm2]

@oznetmaster I think the answer to your questions is, let's wait until someone cares and go from there.
I don't see how 'color' vs 'colour' affects the end user experience one whit, but the team can certainly have that conversation if someone thinks it's important to have. If you think it's important for us to specifically consider a best practice around 'color' vs 'colour' now, let's discuss it and hear what people think.

I think we can have a conversation about whether we want a best practice around end marks without having every other possible conversation.

[oznetmaster]

@jnm2 I think that is fairly shortsighted. "Best practices" just for end marks?

I ask again. Is there an equivalent "best practices" for the documentation pages? If not, I would certainly think that would be a significantly higher priority if consistency is what is being sought here.

[oznetmaster]

For instance, I worked for one company where all the xml comments were in lower case. No caps at the start of the line. Perhaps we need to examine start as well as end issues?

[oznetmaster]

Microsoft makes these recommendations: https://docs.microsoft.com/en-us/dotnet/csharp/codedoc.

Specifically:

Recommendations

Documenting code is recommended for many reasons. What follows are some best practices, general use case scenarios, and things that you should know when using XML documentation tags in your C# code.

-     For the sake of consistency, all publicly visible types and their members should be documented. If you must do it, do it all.
-     Private members can also be documented using XML comments. However, this exposes the inner (potentially confidential) workings of your library.
-     At a bare minimum, types and their members should have a <summary> tag because its content is needed for IntelliSense.
-     Documentation text should be written using complete sentences ending with full stops.
-     Partial classes are fully supported, and documentation information will be concatenated into a single entry for that type.
-     The compiler verifies the syntax of the <exception>, <include>, <param>, <see>, <seealso> and <typeparam> tags.
-     The compiler validates the parameters that contain file paths and references to other parts of the code.
- 

[oznetmaster]

I am happy to agree to recommend these as best practices for NUnit XML documentation, and spend no more time on this issue.

[jnm2]

@nunit/framework-team Okay, so here are the seven guidelines I'd like to discuss:
https://gist.github.com/jnm2/3f230c9f6343d60bb542938dc35e7a96

Q1: Which of these would you like to see as best practices affecting the NUnit Framework product?

Q2: Besides making the best practices document discoverable, do we want to request changes if we notice a PR not following these guidelines? If not, do we want to improve the public XML documentation later on?

Modifications welcome.

[oznetmaster]

Best practices should NEVER affect a PR, or its acceptance!

[oznetmaster]

As I indicated above, I would prefer to adopt a widely circulated and widely used "best practices", as documented by Microsoft in the link above.

[CharliePoole]

@jnm2

Intro paragraph: say what the scope of this is. I would apply it to the framework and engine.api assemblies, which require xml doc on all public methods to avoid compiler errors.

For your first question...

Practice 1: Require end punctuation (not periods) for all sentences. For non sentences, don't use.

Practice 2: I would use these sparingly. We don't need a ref to string or int for example.

Practice 3: I'd add public. Where you say it's OK to remove add: except in assemblies where it causes a compiler error.

Practice 4: Except where it's obvious. In particular, don't use on properties.

Practice 5: Yes

Practice 6: Yes

Practice 7: Be sparing. Don't list exceptions that may be thrown by any or most method calls. I don't like your example here because (1) it lists the info twice and (2) it uses cref for a built-in .NET exception that everyone already knows about. I would only cref our own stuff, if at all.

For your second question...

Do we want to request changes? Well, yes, some of us do anyway. We should all review stuff based on our judgement. If we disagree, we fight it out. It's not possible to say in advance what will look clear and understandable to any of us when we see it, so let's defer to each case.

I want to add that conformity and consistency, beyond the minimum needed for clarity, has no special value to us. Our users are trained programmers and can figure things out.

[CharliePoole]

@oznetmaster The microsoft guideline has seven points. My comments...

1. "For the sake of consistency..." That's not the reason we document. We document so people know how to use our software. The second sentence in this point makes no sense to me.

2, 3, 4. Make sense as guidelines.

5, 6, 7. Are statements about the compiler, not documentation guidelines.

Just taking 2, 3 and 4 makes an acceptable guide for me.

Frankly, I feel no need for such a guideline, but since @jnm2 does and is willing to work on it, I think he should feel free to do it.

When it comes down to reviewing code, anyone is free to make any observation, for any reason at any time. It doesn't require the authority of some document to do that. Since we both make and enforce the rules, we actually only need to write down those things about which we think we will need a reminder in the future.

[oznetmaster]

@CharliePoole Works for me.

[ChrisMaddock]

This looks fine to me, with restricting it to just public types.

[rprouse]

Consistency would be nice, but it is a very old codebase and we have limited time that is better spent fixing bugs and adding functionality. That said, I think we should be looking out for misleading documentation and I also think we should be adding additional information like <see cref="xxx"/> wherever possible to improve the tooling experience.

Also, it is clearly Colour not Color 😝

[oznetmaster]

@ChrisMaddock Which looks fine to you, the Microsoft ones or @jnm2 ones? I really think we should go with the three that @CharliePoole indicated from the Microsoft list.

[ChrisMaddock]

@oznetmaster - Sorry, I didn't realise we were looking at both options.

I have no objection to the more highly specified option @jnm2 proposed. The Microsoft extract appears to be a subset of that.

This isn't something I personally have much interest in attacking - however if @jnm2 does want to bring the current codebase up to this standard, it would undoubtedly be an improvement. That's part of being a non-professional open source project in my eyes, it will always be driven to a certain extent by what the team are interested in working on.

I'm all for writing a 'guidance' document down, and working towards that in the most part - I'd be happy to follow it in my own PRs. I'm less keen on the idea of sending casual contributors back for minor stylistic changes - but I think we touched on that before, and that needs to be handled on a case-by-case basis.

[CharliePoole]

I too could live with either one, always tempered with a bit of pragmatism.

WRT code review, this is an area where I'd like to see changes suggested or recommended, with a justification given that helps people understand rather than just a reference to a document. In the case of coding standards, we just point to the document and say "Just add the underscores like it says." Here, OTOH, we want to say, "It will help users if you do this..."

Some casual contributors will be experienced developers with their own fully-formed notions about how these should be written. We should accept that when it happens, even down to their favorite "colour."

[jnm2]

I have incorporated the feedback into this page: https://github.com/nunit/docs/wiki/Best-practices-for-XML-documentation.

@CharliePoole

it uses cref for a built-in .NET exception that everyone already knows about. I would only cref our own stuff, if at all.

It makes an eye-catching difference every time intellisense or documentation is visible. It's not much effort nor is the raw XML harder than usual to read. I would at least not discourage it if at all possible.

[jnm2]

There were no further comments, so I will consider this finished for now.