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

API documentation debt - System.Collections #43850

Closed
Tracked by #43849
carlossanlop opened this issue Oct 26, 2020 · 6 comments
Closed
Tracked by #43849

API documentation debt - System.Collections #43850

carlossanlop opened this issue Oct 26, 2020 · 6 comments
Assignees
Labels
area-System.Collections documentation Documentation bug or enhancement, does not impact product or test code help wanted [up-for-grabs] Good issue for external contributors
Milestone

Comments

@carlossanlop
Copy link
Member

carlossanlop commented Oct 26, 2020

Area owners: @eiriktsarpalis @layomia

The APIs in the list below are missing some or all of their documentation. Please add the missing documentation directly in triple slash comments in source. We will make sure it gets ported to dotnet-api-docs after it's merged.

Make sure to follow the documentation guidelines defined in the dotnet-api-docs wiki:
https://github.com/dotnet/dotnet-api-docs/wiki

Also please add me as a PR reviewer.

Community contributions are welcome.

System.Collections
DocId Summary Parameters TypeParameters ReturnValue
T:System.Collections.Immutable.ImmutableArray`1.Builder Present NA Missing NA
T:System.Collections.Immutable.ImmutableArray`1.Enumerator Present NA Missing NA
T:System.Collections.Immutable.ImmutableDictionary`2.Builder Present NA Missing NA
T:System.Collections.Immutable.ImmutableDictionary`2.Enumerator Present NA Missing NA
T:System.Collections.Immutable.ImmutableHashSet`1.Builder Present NA Missing NA
T:System.Collections.Immutable.ImmutableHashSet`1.Enumerator Present NA Missing NA
T:System.Collections.Immutable.ImmutableList`1.Builder Present NA Missing NA
T:System.Collections.Immutable.ImmutableList`1.Enumerator Present NA Missing NA
T:System.Collections.Immutable.ImmutableQueue`1.Enumerator Present NA Missing NA
T:System.Collections.Immutable.ImmutableSortedDictionary`2.Builder Present NA Missing NA
T:System.Collections.Immutable.ImmutableSortedDictionary`2.Enumerator Present NA Missing NA
T:System.Collections.Immutable.ImmutableSortedSet`1.Builder Present NA Missing NA
T:System.Collections.Immutable.ImmutableSortedSet`1.Enumerator Present NA Missing NA
T:System.Collections.Immutable.ImmutableStack`1.Enumerator Present NA Missing NA
@carlossanlop carlossanlop added documentation Documentation bug or enhancement, does not impact product or test code help wanted [up-for-grabs] Good issue for external contributors labels Oct 26, 2020
@carlossanlop carlossanlop added this to the 6.0.0 milestone Oct 26, 2020
@Dotnet-GitSync-Bot Dotnet-GitSync-Bot added area-System.Collections untriaged New issue has not been triaged by the area owner labels Oct 26, 2020
@ghost
Copy link

ghost commented Oct 26, 2020

Tagging subscribers to this area: @eiriktsarpalis, @jeffhandley
See info in area-owners.md if you want to be subscribed.

@danmoseley
Copy link
Member

@carlossanlop is there an analyzer or warning setting that would give errors (so clearing out the errors would resolve this issue)?

Is there a preferred ordering of tags, eg., summary, typeparam, remarks?

@carlossanlop
Copy link
Member Author

is there an analyzer or warning setting that would give errors (so clearing out the errors would resolve this issue)?

@danmosemsft Yes, there is a way: https://docs.microsoft.com/en-us/dotnet/csharp/codedoc

But we don't want to enable it yet, it will cause a lot of noise if any public APIs do not have documentation in source, which is the case for a lot of them.

We will first have to port back everything from dotnet-api-docs to triple slash comments and then we will enable that rule. We are working on the plan for this, and we will work on it assembly by assembly. I'll let you know when we are ready to begin.

To answer your question, my suggestion is to simply add the missing documentation from the table. Once we run the DocsPortingTool, it will detect and port only the items that are missing in dotnet-api-docs. Items with existing documentation in that repo will not get their documentation overwritten.

Is there a preferred ordering of tags, eg., summary, typeparam, remarks?

Visual Studio will automatically add the required skeleton if the user types "///" on top of the API to document.

@danmoseley
Copy link
Member

will cause a lot of noise if any public APIs do not have documentation in source

I was just thinking for someone fixing an issue like this, where the source is nearly all documented, they may want to enable it locally so it guides them to the problems.

@eiriktsarpalis eiriktsarpalis removed the untriaged New issue has not been triaged by the area owner label Oct 30, 2020
@TheMaximum
Copy link
Contributor

I was looking to see if I could add the missing typeparams into the code, but appear to have hit a wall.
The issues are the same for all types, I'm taking ImmutableArray{T} as example here.

The mentioned classes are split into multiple files with partial classes. In the case of ImmutableArray{T}, the class summary is added in ImmutableArray_1.Minimal.cs and includes the required typeparam:

/// <summary>
/// A readonly array with O(1) indexable lookup time.
/// </summary>
/// <typeparam name="T">The type of element stored by the array.</typeparam>
/// <devremarks>
/// This type has a documented contract of being exactly one reference-type field in size.
/// Our own <see cref="System.Collections.Immutable.ImmutableInterlocked"/> class depends on it, as well as others externally.
/// IMPORTANT NOTICE FOR MAINTAINERS AND REVIEWERS:
/// This type should be thread-safe. As a struct, it cannot protect its own fields
/// from being changed from one thread while its members are executing on other threads
/// because structs can change *in place* simply by reassigning the field containing
/// this struct. Therefore it is extremely important that
/// ** Every member should only dereference <c>this</c> ONCE. **
/// If a member needs to reference the array field, that counts as a dereference of <c>this</c>.
/// Calling other instance members (properties or methods) also counts as dereferencing <c>this</c>.
/// Any member that needs to use <c>this</c> more than once must instead
/// assign <c>this</c> to a local variable and use that for the rest of the code instead.
/// This effectively copies the one field in the struct to a local variable so that
/// it is insulated from other threads.
/// </devremarks>
[DebuggerDisplay("{DebuggerDisplay,nq}")]
[NonVersionable] // Applies to field layout
public partial struct ImmutableArray<T> : IEnumerable<T>, IEquatable<ImmutableArray<T>>, IImmutableArray
{

When it comes to the Builder and Enumerator classes, the documentation on Docs shows one type parameter (T). When checking in ImmutableArray_1.Builder.cs, the Builder class doesn't have any type parameters. It implements IList{T} and IReadOnlyList{T}, but those type parameters are passed via the type parameter of ImmutableArray{T}.

public partial struct ImmutableArray<T>
{
/// <summary>
/// A writable array accessor that can be converted into an <see cref="ImmutableArray{T}"/>
/// instance without allocating memory.
/// </summary>
[DebuggerDisplay("Count = {Count}")]
[DebuggerTypeProxy(typeof(ImmutableArrayBuilderDebuggerProxy<>))]
public sealed class Builder : IList<T>, IReadOnlyList<T>
{

When I would add the typeparam element to the comment displayed above, I get a CS1711 error (XML comment has a typeparam tag for 'T', but there is no type parameter by that name), which is logical - there isn't.

When adding a comment (including typeparam) to the ImmutableArray{T} class in ImmutableArray_1.Builder.cs, I get a CS1710 error (XML comment has a duplicate typeparam tag for 'T'), which is also logical as the typeparam is already present in ImmutableArray_1.Minimal.cs.


Could you indicate how the documentation gets this typeparam for the Builder/Enumerator classes and what the appropriate action to take would be, @carlossanlop?

@eiriktsarpalis
Copy link
Member

Superseded by #54869.

@ghost ghost locked as resolved and limited conversation to collaborators Sep 23, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Collections documentation Documentation bug or enhancement, does not impact product or test code help wanted [up-for-grabs] Good issue for external contributors
Projects
None yet
Development

No branches or pull requests

5 participants