Port doc comment changes from docs repo by ghogen · Pull Request #7152 · dotnet/msbuild

ghogen · 2021-12-14T20:08:11Z

Context

Doc comments are changed from time to time in the dotnet-api-docs repo. These changes need to be ported back to the MSBuild source, now that the MSBuild source is being used as the source-of-truth for MSBuild reference docs.

Changes Made

Comment-only changes in a number of source files.

Testing

Notes

The changes that were required can also be found here: dotnet/dotnet-api-docs#7484 - this docs PR was necessary to prevent the dotnet-api-docs changes from being overwritten when the comments were re-read from MSBuild sources for the 17.0 version of MSBuild.

src/Framework/Sdk/SdkResolverContext.cs

src/Tasks/CodeTaskFactory.cs

src/Tasks/Copy.cs

Co-authored-by: Rainer Sigwald <raines@microsoft.com>

danmoseley · 2021-12-15T23:57:04Z

@ghogen just curious, do you do this manually or with a tool like https://github.com/carlossanlop/DocsPortingTool ?

ghogen · 2021-12-16T01:06:57Z

@danmoseley I did this manually, but yes the DocsPortingTool is designed to handle this kind of thing. It looked like a bit too much overhead for such a small number of changes.

danmoseley · 2021-12-16T02:56:13Z

@ghogen that's cool, just checking we weren't missing an opportunity to share tech.

Nirmal4G

Note inside remarks have markdown because of auto conversion but we don't need to copy them exactly as markdown. Just plain text is fine. Same as before.

Also, Type crefs are simplified. Some might not have usings defined in the file. Check and add necessary usings if needed!

Nirmal4G · 2021-12-22T06:03:27Z

src/Build/Definition/ProjectCollection.cs

        /// resource), not necessarily the assembly version.
        /// If you want the assembly version, use Constants.AssemblyVersion.
-        /// This is not the <see cref="ToolsetsVersion">ToolsetCollectionVersion</see>.
+        /// This is not the <see cref="P:Microsoft.Build.BuildEngine.ToolsetCollection.ToolsVersions*">ToolsetCollection.ToolsVersions</see>.


Suggested change

/// This is not the <see cref="P:Microsoft.Build.BuildEngine.ToolsetCollection.ToolsVersions*">ToolsetCollection.ToolsVersions</see>.

/// This is not the <see cref="ToolsetCollection.ToolsVersions"/>.

Nirmal4G · 2021-12-22T06:04:13Z

src/Build/Logging/BinaryLogger/BuildEventArgsReader.cs


        /// <summary>
-        /// Initializes a new instance of BuildEventArgsReader using a BinaryReader instance
+        /// Initializes a new instance of <see cref="T:Microsoft.Build.Logging.BuildEventArgsReader"/> using a <see cref="T:System.IO.BinaryReader"/> instance.


Suggested change

/// Initializes a new instance of <see cref="T:Microsoft.Build.Logging.BuildEventArgsReader"/> using a <see cref="T:System.IO.BinaryReader"/> instance.

/// Initializes a new instance of <see cref="BuildEventArgsReader"/> using a <see cref="BinaryReader"/> instance.

Nirmal4G · 2021-12-22T06:04:33Z

src/Build/Logging/BinaryLogger/BuildEventArgsReader.cs

+        /// Initializes a new instance of <see cref="T:Microsoft.Build.Logging.BuildEventArgsReader"/> using a <see cref="T:System.IO.BinaryReader"/> instance.
        /// </summary>
-        /// <param name="binaryReader">The BinaryReader to read BuildEventArgs from</param>
+        /// <param name="binaryReader">The <see cref="T:System.IO.BinaryReader"/> to read <see cref="T:Microsoft.Build.Framework.BuildEventArgs"/> from.</param>


Suggested change

/// <param name="binaryReader">The <see cref="T:System.IO.BinaryReader"/> to read <see cref="T:Microsoft.Build.Framework.BuildEventArgs"/> from.</param>

/// <param name="binaryReader">The <see cref="BinaryReader"/> to read <see cref="BuildEventArgs"/> from.</param>

Nirmal4G · 2021-12-22T06:04:49Z

src/Build/Logging/BinaryLogger/BuildEventArgsReader.cs


        /// <summary>
-        /// Reads the next log record from the binary reader. If there are no more records, returns null.
+        /// Reads the next log record from the <see cref="T:System.IO.BinaryReader"/>.


Suggested change

/// Reads the next log record from the <see cref="T:System.IO.BinaryReader"/>.

/// Reads the next log record from the <see cref="BinaryReader"/>.

Nirmal4G · 2021-12-22T06:05:19Z

src/Build/Logging/BinaryLogger/BuildEventArgsReader.cs

+        /// Reads the next log record from the <see cref="T:System.IO.BinaryReader"/>.
        /// </summary>
+        /// <returns>
+        /// The next <see cref="T:Microsoft.Build.Framework.BuildEventArgs" />. If there are no more records, returns <see langword="null" />.


Suggested change

/// The next <see cref="T:Microsoft.Build.Framework.BuildEventArgs" />. If there are no more records, returns <see langword="null" />.

/// The next <see cref="BuildEventArgs"/>. If there are no more records, returns <see langword="null"/>.

Nirmal4G · 2021-12-22T06:08:41Z

src/Framework/Sdk/SdkResolver.cs

+        ///  <remarks><format type="text/markdown"><![CDATA[
+        ///  ## Remarks
+        ///  > [!NOTE]
+        ///  > You must use the <xref:Microsoft.Build.Framework.SdkResultFactory> to return a result.
+        ///  ]]></format>
+        /// </remarks>


Suggested change

/// <remarks><format type="text/markdown"><![CDATA[

/// ## Remarks

/// > [!NOTE]

/// > You must use the <xref:Microsoft.Build.Framework.SdkResultFactory> to return a result.

/// ]]></format>

/// </remarks>

/// <remarks>

/// Note: You must use the <see cref="SdkResultFactory"/> to return a result.

/// </remarks>

Nirmal4G · 2021-12-22T06:10:34Z

src/Framework/Sdk/SdkResolverContext.cs

+        ///    <format type="text/markdown"><![CDATA[
+        /// ## Remarks
+        ///     
+        /// File version is informational and not equal to the assembly version.
+        /// ]]></format>


Suggested change

/// <format type="text/markdown"><![CDATA[

/// ## Remarks

///

/// File version is informational and not equal to the assembly version.

/// ]]></format>

/// File version is informational and not equal to the assembly version.

Do we need to include info about how the file version is calculated?

Nirmal4G · 2021-12-22T06:13:44Z

src/Framework/Sdk/SdkResult.cs

+    ///    <format type="text/markdown"><![CDATA[
+    /// ## Remarks
+    /// > [!NOTE]
+    /// > Use <xref:Microsoft.Build.Framework.SdkResultFactory> to create instances of this class. Do not inherit from this class.
+    /// ]]></format>


Suggested change

/// <format type="text/markdown"><![CDATA[

/// ## Remarks

/// > [!NOTE]

/// > Use <xref:Microsoft.Build.Framework.SdkResultFactory> to create instances of this class. Do not inherit from this class.

/// ]]></format>

/// Note: Use <see cref="SdkResultFactory"/> to create instances of this class. Do not inherit from this class.

Nirmal4G · 2021-12-22T06:14:41Z

src/Tasks/Hash.cs

+    ///    <format type="text/markdown"><![CDATA[
+    /// ## Remarks
+    /// Currently uses SHA1. The implementation is subject to change between MSBuild versions.
+    /// This class is not intended as a cryptographic security measure, only for uniqueness between build executions.
+    /// ]]></format>


Suggested change

/// <format type="text/markdown"><![CDATA[

/// ## Remarks

/// Currently uses SHA1. The implementation is subject to change between MSBuild versions.

/// This class is not intended as a cryptographic security measure, only for uniqueness between build executions.

/// ]]></format>

/// Currently uses SHA1. The implementation is subject to change between MSBuild versions.

/// This class is not intended as a cryptographic security measure, only for uniqueness between build executions.

elachlan · 2022-01-03T20:58:34Z

Note inside remarks have markdown because of auto conversion but we don't need to copy them exactly as markdown. Just plain text is fine. Same as before.

Also, Type crefs are simplified. Some might not have usings defined in the file. Check and add necessary usings if needed!

We have a PR for the cref changes at #7194, do we want to merge that first and then rebase this PR?

Nirmal4G · 2022-01-06T05:34:01Z

@elachlan This is merged, so add these suggestions to your cref PR.

Forgind · 2022-01-06T15:28:59Z

Do we have somewhere we said we want short crefs? I had no idea, but rainersigwald said no.

Forgind · 2022-01-06T15:29:24Z

If we'd originally listed it as info, that may have been a mistake on our part—if so, sorry about that!

rainersigwald · 2022-01-06T15:49:58Z

I don't feel strongly about the short/long crefs. I do feel strongly that that style issue shouldn't block this change to reduce pain on our docs partners.

elachlan · 2022-01-07T08:36:22Z

@ghogen is the markdown important in this issue? Nirmal's suggestions have been applied in another PR and I wanted to make sure whether or not the markdown was required and needed to be preserved.

ghogen · 2022-01-07T17:08:34Z

@elachlan @Nirmal4G
The Markdown is used in the dotnet-api-docs repo, but it's only required if there's special formatting such as for a Note/Caution/Tip construct for example. So I think it's OK to implement those suggestions in the other PR. Note also, we're doing work now to separate the MSBuild API reference from the .NET reference, which will give us more flexibility, because we can control the process better to avoid changes that occur in multiple repos (which is what led to my PR).

Nirmal4G · 2022-01-08T01:11:00Z

@ghogen

The Markdown is used in the dotnet-api-docs repo, but it's only required if there's special formatting such as for a Note/Caution/Tip construct…

The XML Doc comments already includes all these in the remarks element but as plain-text with a heading indicating the intent. Wouldn't it be possible for the doc engine to take it and convert to markdown and convert back to plain-text as is? That way, we wouldn't need those CDATA elements in the XML Doc.

port comment changes

258da8d

ghogen changed the title ~~port comment changes~~ Port doc comment changes from docs repo Dec 14, 2021

fix comment marker

31162d7

rainersigwald approved these changes Dec 14, 2021

View reviewed changes

src/Framework/Sdk/SdkResolverContext.cs Outdated Show resolved Hide resolved

src/Tasks/CodeTaskFactory.cs Outdated Show resolved Hide resolved

src/Tasks/Copy.cs Outdated Show resolved Hide resolved

src/Tasks/Copy.cs Outdated Show resolved Hide resolved

Apply suggestions from code review

42732af

Co-authored-by: Rainer Sigwald <raines@microsoft.com>

Nirmal4G suggested changes Dec 22, 2021

View reviewed changes

Forgind added the merge-when-branch-open PRs that are approved, except that there is a problem that means we are not merging stuff right now. label Jan 5, 2022

Forgind merged commit 84b30e7 into dotnet:main Jan 5, 2022

Nirmal4G mentioned this pull request Jan 6, 2022

CA1200: Avoid using cref tags with a prefix #7194

Merged

	/// This is not the <see cref="P:Microsoft.Build.BuildEngine.ToolsetCollection.ToolsVersions*">ToolsetCollection.ToolsVersions</see>.
	/// This is not the <see cref="ToolsetCollection.ToolsVersions"/>.

	/// Initializes a new instance of <see cref="T:Microsoft.Build.Logging.BuildEventArgsReader"/> using a <see cref="T:System.IO.BinaryReader"/> instance.
	/// Initializes a new instance of <see cref="BuildEventArgsReader"/> using a <see cref="BinaryReader"/> instance.

	/// <param name="binaryReader">The <see cref="T:System.IO.BinaryReader"/> to read <see cref="T:Microsoft.Build.Framework.BuildEventArgs"/> from.</param>
	/// <param name="binaryReader">The <see cref="BinaryReader"/> to read <see cref="BuildEventArgs"/> from.</param>

	/// Reads the next log record from the <see cref="T:System.IO.BinaryReader"/>.
	/// Reads the next log record from the <see cref="BinaryReader"/>.

	/// The next <see cref="T:Microsoft.Build.Framework.BuildEventArgs" />. If there are no more records, returns <see langword="null" />.
	/// The next <see cref="BuildEventArgs"/>. If there are no more records, returns <see langword="null"/>.

Conversation

ghogen commented Dec 14, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Context

Changes Made

Testing

Notes

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

danmoseley commented Dec 15, 2021

Uh oh!

ghogen commented Dec 16, 2021

Uh oh!

danmoseley commented Dec 16, 2021

Uh oh!

Nirmal4G left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elachlan commented Jan 3, 2022

Uh oh!

Nirmal4G commented Jan 6, 2022

Uh oh!

Forgind commented Jan 6, 2022

Uh oh!

Forgind commented Jan 6, 2022

Uh oh!

rainersigwald commented Jan 6, 2022

Uh oh!

elachlan commented Jan 7, 2022

Uh oh!

ghogen commented Jan 7, 2022

Uh oh!

Nirmal4G commented Jan 8, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

ghogen commented Dec 14, 2021 •

edited

Loading