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

Backport System.Linq API docs to source #51140

Closed
wants to merge 6 commits into from

Conversation

eiriktsarpalis
Copy link
Member

@eiriktsarpalis eiriktsarpalis commented Apr 12, 2021

This is a first-pass attempt at backporting System.Linq API docs to source code. I did come across a few issues during the backporting effort and a substantial part of the xml docs required manual editing in order to satisfy the compiler:

  • I encountered problems backporting xref nodes with document id's to general method pages. The porting tool will simply map to a cref and trim the trailing asterisk, however to my knowledge cref attributes do not have an equivalent concept and a specific method overload will need to be selected. This is problematic since links will now be pointing to a specific method overload, in most cases contrary to the intentions of the docs. It also makes the backporting process more labor intensive and error prone.
  • cref attributes cannot be used to point to types outside of the current project (or System.Private.CoreLib). One example I encountered was System.Linq.IQueryable<T> (in System.Linq.Expressions.dll) having xref links to System.Linq.Queryable (in System.Linq.Queryable.dll) which cannot be ported to cref links. I worked around the issue by backticking the method names, however this means that the xref links will be lost whenever we port the source docs back to dotnet-api-docs.
  • I came across a bug in the docId to cref converter of the porting tool when it comes to methods with multiple generic parameters (Linq has loads of those!). I've posted a PR that fixes the bug fix mapping of generic xref's to cref's api-docs-sync#72.

@carlossanlop what is the recommended course of action for the first two issues? It feels to me that in both cases we should abandon cref's and use xref:docId directly. Obviously making the tool support this OOTB would simplify the porting effort significantly.

cc @jeffhandley

Fix #44969.

It looks like the first two points are addressed by this document. Thanks to @jeffhandley for bringing it up.

@ghost
Copy link

ghost commented Apr 12, 2021

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

Issue Details

This is a first-pass attempt at backporting System.Linq API docs to source code. I did come across a few issues during the backporting effort and a substantial part of the xml docs required manual editing in order to satisfy the compiler:

  • I encountered problems backporting xref nodes with document id's to general method pages. The porting tool will simply map to a cref and trim the trailing asterisk, however to my knowledge cref attributes do not have an equivalent concept and a specific method overload will need to be selected. This is problematic since links will now be pointing to a specific method overload, in most cases contrary to the intentions of the docs. It also makes the backporting process more labor intensive and error prone.
  • cref attributes cannot be used to point to types outside of the current project (or System.Private.CoreLib). One example I encountered was System.Linq.IQueryable<T> (in System.Linq.Expressions.dll) having xref links to System.Linq.Queryable (in System.Linq.Queryable.dll) which cannot be ported to cref links. I worked around the issue by backticking the method names, however this means that the xref links will be lost whenever we port the source docs back to dotnet-api-docs.
  • I came across a bug in the docId to cref converter of the porting tool when it comes to methods with multiple generic parameters (Linq has loads of those!). I've posted a PR that fixes the bug fix mapping of generic xref's to cref's api-docs-sync#72.

@carlossanlop what is the recommended course of action for the first two issues? It feels to me that in both cases we should abandon cref's and use xref:docId directly. Obviously making the tool support this OOTB would simplify the porting effort significantly.

cc @jeffhandley

Fix #44969.

Author: eiriktsarpalis
Assignees: eiriktsarpalis
Labels:

area-System.Linq

Milestone: -

@jeffhandley jeffhandley changed the title Backlog System.Linq API docs to source Backport System.Linq API docs to source Apr 14, 2021
Copy link
Member

@jeffhandley jeffhandley left a comment

Choose a reason for hiding this comment

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

I've only skimmed through about 10% of the review so far, but I see a few patterns that I wasn't sure about.

/// :::code language="csharp" source="~/samples/snippets/csharp/VS_Snippets_CLR_System/system.Linq.Expressions.Expression/CS/Expression.cs" id="Snippet8":::
/// :::code language="vb" source="~/samples/snippets/visualbasic/VS_Snippets_CLR_System/system.Linq.Expressions.Expression/VB/Expression.vb" id="Snippet8":::</example>
Copy link
Member

Choose a reason for hiding this comment

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

I think the :::code syntax is incorrect; @carlossanlop to verify though. I believe these need to be converted to [!code-csharp] syntax.

If so, there are a lot of these throughout the PR and should be fixed in the porting tool.

Copy link
Member Author

Choose a reason for hiding this comment

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

Some of these blocks define additional attributes, for example interactive: https://github.com/dotnet/dotnet-api-docs/blob/main/xml/System.Linq/Enumerable.xml#L117

It's not clear to me how that could be mapped to code-csharp syntax accurately. Any hints @carlossanlop?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hopefully we can leave the syntax as :::code because @mairaw just converted it all a couple weeks ago. Is there a problem leaving it in that format? That way we preserve interactive info too.

Copy link
Member

Choose a reason for hiding this comment

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

The tool detected an ## Example header and separated the code examples from the remarks section. What the tool didn't do, is detect the new :::code sections, which should've been preserved inside a markdown section.

@gewarren @mairaw correct me if I'm wrong: in dotnet-api-docs, do the :::code sections only work if they are inside a remarks section? Or do they also work if they are in plain xml, like here?

Copy link
Contributor

Choose a reason for hiding this comment

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

I believe the ::: syntax is Markdown-specific. @tdykstra is there a way to specify interactive with ![]() style code snippets? I don't even see that syntax mentioned in the contributor guide anymore.

Choose a reason for hiding this comment

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

There is, but I believe the old format is Markdown-specific also. You can go a year or so back into the git history of the code-in-docs article to find the old format documented.

Copy link
Contributor

Choose a reason for hiding this comment

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

Found it:

[!code-csharp-interactive[](path/from/repo/root/Example.cs)]

Copy link
Contributor

@mairaw mairaw Apr 27, 2021

Choose a reason for hiding this comment

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

Just FYI that format doesn't work for all kinds of interactive options, that's why I had to convert to the new format.
That format defaults to interactive="try-dotnet-method", so you're losing information if you convert.

@eiriktsarpalis eiriktsarpalis force-pushed the backport-linq-docs branch 2 times, most recently from f46513e to bf40588 Compare April 16, 2021 10:24
@eiriktsarpalis eiriktsarpalis requested a review from gewarren April 26, 2021 16:48
Copy link
Contributor

@gewarren gewarren left a comment

Choose a reason for hiding this comment

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

Did we decide the ::: code snippets are okay? For the things I commented on, I noticed them throughout the diff, not just in that one spot.

/// <remarks>The <see cref="System.Linq.IQueryable" /> interface is intended for implementation by query providers. It is only supposed to be implemented by providers that also implement <see cref="System.Linq.IQueryable{T}" />. If the provider does not also implement <see cref="System.Linq.IQueryable{T}" />, the standard query operators cannot be used on the provider's data source.
/// The <see cref="System.Linq.IQueryable" /> interface inherits the <see cref="System.Collections.IEnumerable" /> interface so that if it represents a query, the results of that query can be enumerated. Enumeration causes the expression tree associated with an <see cref="System.Linq.IQueryable" /> object to be executed. The definition of "executing an expression tree" is specific to a query provider. For example, it may involve translating the expression tree to an appropriate query language for the underlying data source. Queries that do not return enumerable results are executed when the <see cref="O:System.Linq.IQueryProvider.Execute" /> method is called.
/// For more information about how to create your own LINQ provider, see <a href="https://docs.microsoft.com/archive/blogs/mattwar/linq-building-an-iqueryable-provider-part-i">LINQ: Building an IQueryable Provider</a>.</remarks>
/// <altmember langword="System.Linq.Queryable"/>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// <altmember langword="System.Linq.Queryable"/>
/// <altmember cref="System.Linq.Queryable"/>

Copy link
Contributor

Choose a reason for hiding this comment

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

Was this introduced by the tool? If so seems like a bug.

/// <summary>Provides functionality to evaluate queries against a specific data source wherein the type of the data is not specified.</summary>
/// <remarks>The <see cref="System.Linq.IQueryable" /> interface is intended for implementation by query providers. It is only supposed to be implemented by providers that also implement <see cref="System.Linq.IQueryable{T}" />. If the provider does not also implement <see cref="System.Linq.IQueryable{T}" />, the standard query operators cannot be used on the provider's data source.
/// The <see cref="System.Linq.IQueryable" /> interface inherits the <see cref="System.Collections.IEnumerable" /> interface so that if it represents a query, the results of that query can be enumerated. Enumeration causes the expression tree associated with an <see cref="System.Linq.IQueryable" /> object to be executed. The definition of "executing an expression tree" is specific to a query provider. For example, it may involve translating the expression tree to an appropriate query language for the underlying data source. Queries that do not return enumerable results are executed when the <see cref="O:System.Linq.IQueryProvider.Execute" /> method is called.
/// For more information about how to create your own LINQ provider, see <a href="https://docs.microsoft.com/archive/blogs/mattwar/linq-building-an-iqueryable-provider-part-i">LINQ: Building an IQueryable Provider</a>.</remarks>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// For more information about how to create your own LINQ provider, see <a href="https://docs.microsoft.com/archive/blogs/mattwar/linq-building-an-iqueryable-provider-part-i">LINQ: Building an IQueryable Provider</a>.</remarks>
/// For more information about how to create your own LINQ provider, see <see href="https://docs.microsoft.com/archive/blogs/mattwar/linq-building-an-iqueryable-provider-part-i">LINQ: Building an IQueryable Provider</see>.</remarks>

Copy link
Contributor

Choose a reason for hiding this comment

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

Should be fixed in tool if necessary.

/// </summary>
/// <summary>Gets the type of the element(s) that are returned when the expression tree associated with this instance of <see cref="System.Linq.IQueryable" /> is executed.</summary>
/// <value>A <see cref="System.Type" /> that represents the type of the element(s) that are returned when the expression tree associated with this object is executed.</value>
/// <remarks>The <see cref="O:System.Linq.IQueryable.ElementType" /> property represents the "T" in `IQueryable&lt;T&gt;` or `IQueryable(Of T)`.</remarks>
Copy link
Contributor

Choose a reason for hiding this comment

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

To use the backticks here, it would need to be in a markdown CDATA section. Otherwise use <code>.

/// </summary>
/// <summary>Gets the type of the element(s) that are returned when the expression tree associated with this instance of <see cref="System.Linq.IQueryable" /> is executed.</summary>
/// <value>A <see cref="System.Type" /> that represents the type of the element(s) that are returned when the expression tree associated with this object is executed.</value>
/// <remarks>The <see cref="O:System.Linq.IQueryable.ElementType" /> property represents the "T" in `IQueryable&lt;T&gt;` or `IQueryable(Of T)`.</remarks>
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// <remarks>The <see cref="O:System.Linq.IQueryable.ElementType" /> property represents the "T" in `IQueryable&lt;T&gt;` or `IQueryable(Of T)`.</remarks>
/// <remarks>The <see cref="O:System.Linq.IQueryable.ElementType" /> property represents the "T" in <code>IQueryable&lt;T&gt;</code> or <code>IQueryable(Of T)</code>.</remarks>

/// This interface inherits the <see cref="System.Collections.Generic.IEnumerable{T}" /> interface so that if it represents a query, the results of that query can be enumerated. Enumeration forces the expression tree associated with an <see cref="System.Linq.IQueryable{T}" /> object to be executed. Queries that do not return enumerable results are executed when the <see cref="System.Linq.IQueryProvider.Execute{T}(System.Linq.Expressions.Expression)" /> method is called.
/// The definition of "executing an expression tree" is specific to a query provider. For example, it may involve translating the expression tree to a query language appropriate for an underlying data source.
/// The <see cref="System.Linq.IQueryable{T}" /> interface enables queries to be polymorphic. That is, because a query against an `IQueryable` data source is represented as an expression tree, it can be executed against different types of data sources.
/// The <see langword="static" /> (`Shared` in Visual Basic) methods defined in the class <see langword="System.Linq.Queryable" /> (except for <see cref="O:System.Linq.Queryable.AsQueryable" />, <see cref="O:System.Linq.Queryable.ThenBy" />, and <see cref="O:System.Linq.Queryable.ThenByDescending" />) extend objects of types that implement the <see cref="System.Linq.IQueryable{T}" /> interface.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// The <see langword="static" /> (`Shared` in Visual Basic) methods defined in the class <see langword="System.Linq.Queryable" /> (except for <see cref="O:System.Linq.Queryable.AsQueryable" />, <see cref="O:System.Linq.Queryable.ThenBy" />, and <see cref="O:System.Linq.Queryable.ThenByDescending" />) extend objects of types that implement the <see cref="System.Linq.IQueryable{T}" /> interface.
/// The <see langword="static" /> (<see langword="Shared" /> in Visual Basic) methods defined in the class <see langword="System.Linq.Queryable" /> (except for <see cref="O:System.Linq.Queryable.AsQueryable" />, <see cref="O:System.Linq.Queryable.ThenBy" />, and <see cref="O:System.Linq.Queryable.ThenByDescending" />) extend objects of types that implement the <see cref="System.Linq.IQueryable{T}" /> interface.

/// <returns>An <see cref="IOrderedEnumerable{T}" /> whose elements are sorted according to a key.</returns>
/// <exception cref="System.ArgumentNullException"><paramref name="source" /> or <paramref name="keySelector" /> is <see langword="null" />.</exception>
/// <remarks>This method is implemented by using deferred execution. The immediate return value is an object that stores all the information that is required to perform the action. The query represented by this method is not executed until the object is enumerated either by calling its `GetEnumerator` method directly or by using `foreach` in Visual C# or `For Each` in Visual Basic.
/// To order a sequence by the values of the elements themselves, specify the identity function (`x => x` in Visual C# or `Function(x) x` in Visual Basic) for <paramref name="keySelector" />.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// To order a sequence by the values of the elements themselves, specify the identity function (`x => x` in Visual C# or `Function(x) x` in Visual Basic) for <paramref name="keySelector" />.
/// To order a sequence by the values of the elements themselves, specify the identity function (<code>x => x</code> in C# or <code>Function(x) x</code> in Visual Basic) for <paramref name="keySelector" />.

@eiriktsarpalis
Copy link
Member Author

Did we decide the ::: code snippets are okay? For the things I commented on, I noticed them throughout the diff, not just in that one spot.

Yes they are pretty pervasive. I'm not entirely sure what the best course of action should be.

@jeffhandley jeffhandley marked this pull request as draft May 22, 2021 09:55
@ghost ghost closed this Jun 21, 2021
@ghost
Copy link

ghost commented Jun 21, 2021

Draft Pull Request was automatically closed for inactivity. Please let us know if you'd like to reopen it.

@ghost ghost locked as resolved and limited conversation to collaborators Jul 21, 2021
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Libraries APIs are fully documented using efficient workflows
6 participants