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 docs for System.Diagnostics.Process #48137

Closed
wants to merge 18 commits into from

Conversation

carlossanlop
Copy link
Member

@carlossanlop carlossanlop commented Feb 11, 2021

Backporting public API documentation from MS Docs into triple slash comments for the System.Diagnostics.Process assembly, which includes the System.Diagnostics and Microsoft.Win32.SafeHandles namespaces.

This namespace was interesting for this experiment because some types had cross-platform implementations. This meant the tool had to find all the possible compilations for the project (one for each platform) and make sure the exact same documentation was placed on top of all the different partial implementations.

It was also interesting to find so many remarks that contained elements that couldn't be converted from markdown to xml, so the tool had to detect those cases and keep the CDATA section unmodified.

The full description of the purpose of this PR is described in the issue #44969 - Pilot a new process that extracts API documentation from source code under the section "Bring documentation from Docs to triple slash".

I ran the porting tool using this command:

DocsPortingTool.exe \
-CsProj D:\runtime\src\libraries\System.Diagnostics.Process\src\System.Diagnostics.Process.csproj
-Docs D:\dotnet-api-docs\xml
-Direction ToTripleSlash
-IncludedAssemblies System.Diagnostics.Process
-IncludedNamespaces System.Diagnostics,Microsoft.Win32.SafeHandles

Please compare the ported docs with their source files in dotnet-api-docs:

@ghost
Copy link

ghost commented Feb 11, 2021

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

Issue Details

Backporting public API documentation from MS Docs into triple slash comments for the System.Diagnostics.Process assembly, which includes the System.Diagnostics and Microsoft.Win32.SafeHandles namespaces.

This namespace was interesting for this experiment because some types had cross-platform implementations. This meant the tool had to find all the possible compilations for the project (one for each platform) and make sure the exact same documentation was placed on top of all the different partial implementations.

The full description of the purpose of this PR is described in the issue #44969 - Pilot a new process that extracts API documentation from source code under the section "Bring documentation from Docs to triple slash".

I ran the porting tool using this command:

DocsPortingTool.exe \
-CsProj D:\runtime\src\libraries\System.Diagnostics.Process\src\System.Diagnostics.Process.csproj
-Docs D:\dotnet-api-docs\xml
-Save true
-Direction ToTripleSlash
-IncludedAssemblies System.Diagnostics.Process
-IncludedNamespaces System.Diagnostics,Microsoft.Win32.SafeHandles

Please compare the ported docs with their source files in dotnet-api-docs:

Author: carlossanlop
Assignees: carlossanlop
Labels:

area-System.Diagnostics.Process

Milestone: 6.0.0

Comment on lines +40 to +42
/// <summary>Closes the handle.</summary>
/// <returns>On Windows, returns <see langword="true" /> if the handle is closed successfully; <see langword="false" /> otherwise. On Unix, always returns <see langword="true" /></returns>
/// <remarks>On Windows, to get extended error information, call <see cref="System.Runtime.InteropServices.Marshal.GetLastWin32Error()" />.</remarks>
Copy link
Member Author

Choose a reason for hiding this comment

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

@carlossanlop carlossanlop marked this pull request as ready for review February 11, 2021 20:54
@carlossanlop carlossanlop changed the title [WIP] Backport docs for System.Diagnostics.Process Backport docs for System.Diagnostics.Process Feb 11, 2021
/// <remarks>This property is referenced if the process is being started by using the user name, password, and domain.
/// If the value is <see langword="true" />, the user's profile in the `HKEY_USERS` registry key is loaded. Loading the profile can be time-consuming. Therefore, it is best to use this value only if you must access the information in the `HKEY_CURRENT_USER` registry key.
/// In Windows Server 2003 and Windows 2000, the profile is unloaded after the new process has been terminated, regardless of whether the process has created child processes.
/// In Windows XP, the profile is unloaded after the new process and all child processes it has created have been terminated.</remarks>
Copy link
Member

Choose a reason for hiding this comment

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

There is no particular need to clean up the docs while we do this, of course. But it is funny to see mention of things as old as Windows XP in here. I guess in general, we expect there to be comments like this in our code that do not apply to .NET Core at all?

(And what about comments on API that we simply don't have any more for some reason, that's got to be very rare of course.)

Copy link
Member Author

Choose a reason for hiding this comment

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

we expect there to be comments like this in our code that do not apply to .NET Core at all?

Yes, I'm porting everything unmodified. But also keep in mind that MS Docs had one single docs section for APIs that exist in both .NET Core and .NET Framework. This particular API you pointed out exists in both versions, so it makes sense to preserve this comment.

For APIs that only exist in .NET Framework, we decided their source of truth will be MS Docs, since we won't be able to backport it to source.

/// <remarks><format type="text/markdown"><![CDATA[
/// The <xref:System.Diagnostics.ProcessStartInfo.Verbs%2A> property enables you to determine the verbs that can be used with the file specified by the <xref:System.Diagnostics.ProcessStartInfo.FileName%2A> property. You can set the <xref:System.Diagnostics.ProcessStartInfo.Verb%2A> property to the value of any verb in the set. Examples of verbs are "Edit", "Open", "OpenAsReadOnly", "Print", and "Printto".
/// When you use the <xref:System.Diagnostics.ProcessStartInfo.Verbs%2A> property, you must include the file name extension when you set the value of the <xref:System.Diagnostics.ProcessStartInfo.FileName%2A> property. The file name extension determines the set of possible verbs.
/// ## Examples
Copy link
Member

Choose a reason for hiding this comment

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

Same question as below -- is there/will there be any kind of tooling validatoin of the markdown in these CDATA blocks?

Copy link
Member Author

Choose a reason for hiding this comment

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

Since it's being backported without changes from MS Docs, it has already been validated there.

We do not have plans to have markdown sections validated on the source side. Do you have strong opinions on getting validation? This would require opening a bug in the csharplang repo to request this feature, right?

Copy link
Member

Choose a reason for hiding this comment

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

No I don't have a strong opinion. I doubt the compiler is ever going to validate MD/XML mixtures. I was more wondering whether we are setting ourselves up for success by mixing two markdown languages together and whether there is a path out of that. Clearly the api docs repo chose to do that but we have the luxury of time to be sure we have the right model.

@carlossanlop
Copy link
Member Author

@gewarren @joelmartinez do you know if mdoc knows how to interpret an <example> element? Can it be properly converted to something meaningful in the MS Docs xmls?

If it can be handled correctly, then in triple slash comments, we could separate remarks from examples, which would allow us to save the remarks as pure xml in more cases, while the examples would be saved as markdown:

We do this now

/// <remarks><format type="text/markdown"><![CDATA[
///
/// These are the remarks with a <xref:System.IO.File>.
///
/// ## Example
///
/// ```cs
/// File = new File();
/// ```
/// 
/// ]]></format></remarks>

Do this instead

/// <remarks>These are the remarks with a <see cref="System.IO.File" />.</remarks>
/// <example><format type="text/markdown"><![CDATA[
/// ```cs
/// File = new File();
/// ```
/// ]]></format></example>

@danmoseley
Copy link
Member

Do this instead

That makes more sense to me - remarks in regular XML, with CDATA for code fenced blocks. As it is, in the Compression PR for example, I see a mixture of <xref:System.IO.IOException> and cref="System.IO.IOException"> .. we aren't going to get it right going forward, not just because it's a mixture but also because the compiler can't validate the xref part.

@carlossanlop
Copy link
Member Author

@danmoseley I'm tempted to just do the change in DocsPortingTool to split examples this way, and at the same time make the request for the Docs dev team to address this in mdoc when we start sending them intellisense xml files containing <example> sections. I doubt we can handle them today.

@gewarren @joelmartinez @TianqiZhang @mimisasouvanh FYI

@gewarren
Copy link
Contributor

Do this instead

That makes more sense to me - remarks in regular XML, with CDATA for code fenced blocks. As it is, in the Compression PR for example, I see a mixture of <xref:System.IO.IOException> and cref="System.IO.IOException"> .. we aren't going to get it right going forward, not just because it's a mixture but also because the compiler can't validate the xref part.

The <example> tag does render an "Examples" heading. Here are a couple of "examples":

I'm wondering if we even need the CDATA for the example though. Can we just use <code> tags?

@carlossanlop
Copy link
Member Author

Thanks for confirming, @gewarren ! I will try to use <code> as you're suggesting, instead of CDATA. When the Docs CI starts working again, I'll submit a test PR to confirm it renders fine.

@carlossanlop
Copy link
Member Author

@danmoseley I updated the tool so that it ports remarks as xml, but only the specific sections that cannot be translated to xml, are wrapped with cdata. I updated this PR to reflect those changes.

Copy link
Member

@adamsitnik adamsitnik left a comment

Choose a reason for hiding this comment

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

This was... a LOT of changes! I am really happy to see the old comments gone! Great work @carlossanlop 👍

I did not find any blocking issues, however, it would be nice if you could:

  • fix|remove the dead links
  • consider implementing word-wrap for the tool
  • consider using a bot account for this kind of PRs (you are de facto becoming an owner of 20% of the code? FWIW I am fine with that, but in the past, we have used bots for similar things)

/// <altmember cref="System.Diagnostics.Process.CloseMainWindow"/>
/// <altmember cref="O:System.Diagnostics.Process.Kill"/>
/// <altmember cref="System.Diagnostics.ProcessThread"/>
/// <related type="ExternalDocumentation" href="https://code.msdn.microsoft.com/windowsdesktop/Using-the-NET-Process-Class-d70597ef">Using the .NET Process Class</related>
Copy link
Member

Choose a reason for hiding this comment

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

@@ -226,6 +236,16 @@ private string GetMainWindowTitle()
return title.Slice(0, length).ToString();
}

/// <summary>Gets the window handle of the main window of the associated process.</summary>
/// <value>The system-generated window handle of the main window of the associated process.</value>
/// <remarks>The main window is the window opened by the process that currently has the focus (the <see langword="System.Windows.Forms.Form.TopLevel" /> form). You must use the <see cref="System.Diagnostics.Process.Refresh" /> method to refresh the <see cref="System.Diagnostics.Process" /> object to get the most up to date main window handle if it has changed. In general, because the window handle is cached, use <see cref="System.Diagnostics.Process.Refresh" /> beforehand to guarantee that you'll retrieve the current handle.
Copy link
Member

Choose a reason for hiding this comment

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

this line contains more than 500 characters. Should we consider applying some kind of word-wrap?

/// <altmember cref="System.Diagnostics.Process.CloseMainWindow"/>
/// <altmember cref="O:System.Diagnostics.Process.Kill"/>
/// <altmember cref="System.Diagnostics.ProcessThread"/>
/// <related type="ExternalDocumentation" href="https://code.msdn.microsoft.com/windowsdesktop/Using-the-NET-Process-Class-d70597ef">Using the .NET Process Class</related>
Copy link
Member

Choose a reason for hiding this comment

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

dead link

/// <altmember cref="System.Diagnostics.Process.CloseMainWindow"/>
/// <altmember cref="O:System.Diagnostics.Process.Kill"/>
/// <altmember cref="System.Diagnostics.ProcessThread"/>
/// <related type="ExternalDocumentation" href="https://code.msdn.microsoft.com/windowsdesktop/Using-the-NET-Process-Class-d70597ef">Using the .NET Process Class</related>
Copy link
Member

Choose a reason for hiding this comment

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

dead link

Comment on lines +107 to +111
/// ArgumentList = {
/// "/c",
/// "dir",
/// @"C:\Program Files\dotnet"
/// }
Copy link
Member

Choose a reason for hiding this comment

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

does code samples provided in the comments support indentation?

Suggested change
/// ArgumentList = {
/// "/c",
/// "dir",
/// @"C:\Program Files\dotnet"
/// }
/// ArgumentList = {
/// "/c",
/// "dir",
/// @"C:\Program Files\dotnet"
/// }

/// operating system. Use extreme care when using the high-priority class, because
/// a high-priority class application can use nearly all available CPU time.
/// </devdoc>
/// <summary>Specifies that the process performs time-critical tasks that must be executed immediately, such as the <see langword="Task List" /> dialog, which must respond quickly when called by the user, regardless of the load on the operating system. The threads of the process preempt the threads of normal or idle priority class processes. <br />Use extreme care when specifying <see langword="High" /> for the process's priority class, because a high priority class application can use nearly all available processor time.</summary>
Copy link
Member

Choose a reason for hiding this comment

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

it's one of the places where having a word-wrap would be really nice

/// |0x0004|3|
/// |0x0005|1 or 3|
/// |0x0007|1, 2, or 3|
/// |0x000F|1, 2, 3, or 4|
Copy link
Member

Choose a reason for hiding this comment

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

just for my education: is VS smart enough to format this table properly?

@@ -37,6 +37,9 @@ public sealed partial class SafeProcessHandle : SafeHandleZeroOrMinusOneIsInvali

internal int ProcessId { get; }

/// <summary>Closes the handle.</summary>
/// <returns>On Windows, returns <see langword="true" /> if the handle is closed successfully; <see langword="false" /> otherwise. On Unix, always returns <see langword="true" /></returns>
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
/// <returns>On Windows, returns <see langword="true" /> if the handle is closed successfully; <see langword="false" /> otherwise. On Unix, always returns <see langword="true" /></returns>
/// <returns>On Windows, returns <see langword="true" /> if the handle is closed successfully; <see langword="false" /> otherwise. On Unix, always returns <see langword="true" />.</returns>

public sealed partial class SafeProcessHandle : SafeHandleZeroOrMinusOneIsInvalid
{
/// <summary>Closes the handle.</summary>
/// <returns>On Windows, returns <see langword="true" /> if the handle is closed successfully; <see langword="false" /> otherwise. On Unix, always returns <see langword="true" /></returns>
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
/// <returns>On Windows, returns <see langword="true" /> if the handle is closed successfully; <see langword="false" /> otherwise. On Unix, always returns <see langword="true" /></returns>
/// <returns>On Windows, returns <see langword="true" /> if the handle is closed successfully; <see langword="false" /> otherwise. On Unix, always returns <see langword="true" />.</returns>

/// <altmember cref="System.Diagnostics.Process.CloseMainWindow"/>
/// <altmember cref="O:System.Diagnostics.Process.Kill"/>
/// <altmember cref="System.Diagnostics.ProcessThread"/>
/// <related type="ExternalDocumentation" href="https://code.msdn.microsoft.com/windowsdesktop/Using-the-NET-Process-Class-d70597ef">Using the .NET Process Class</related>
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
/// <related type="ExternalDocumentation" href="https://code.msdn.microsoft.com/windowsdesktop/Using-the-NET-Process-Class-d70597ef">Using the .NET Process Class</related>

Copy link
Contributor

Choose a reason for hiding this comment

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

Bad link.

/// -or-
/// There is no process associated with this <see cref="System.Diagnostics.Process" /> object.
/// -or-
/// The calling process is a member of the associated process' descendant tree.</exception>
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 calling process is a member of the associated process' descendant tree.</exception>
/// The calling process is a member of the associated process's descendant tree.</exception>

/// There is no process associated with this <see cref="System.Diagnostics.Process" /> object.
/// -or-
/// The calling process is a member of the associated process' descendant tree.</exception>
/// <exception cref="System.AggregateException">Not all processes in the associated process' descendant tree could be terminated.</exception>
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
/// <exception cref="System.AggregateException">Not all processes in the associated process' descendant tree could be terminated.</exception>
/// <exception cref="System.AggregateException">Not all processes in the associated process's descendant tree could be terminated.</exception>

/// If you have just started a process and want to use its main window title, consider using the <see cref="O:System.Diagnostics.Process.WaitForInputIdle" /> method to allow the process to finish starting, ensuring that the main window handle has been created. Otherwise, the system throws an exception.
/// <format type="text/markdown"><![CDATA[
/// > [!NOTE]
/// > The main window is the window that currently has the focus; note that this might not be the primary window for the process. You must use the <xref:System.Diagnostics.Process.Refresh%2A> method to refresh the <xref:System.Diagnostics.Process> object to get the most up to date main window handle if it has changed.
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 main window is the window that currently has the focus; note that this might not be the primary window for the process. You must use the <xref:System.Diagnostics.Process.Refresh%2A> method to refresh the <xref:System.Diagnostics.Process> object to get the most up to date main window handle if it has changed.
/// > The main window is the window that currently has the focus. This might not be the primary window for the process. Use the <xref:System.Diagnostics.Process.Refresh%2A> method to refresh the <xref:System.Diagnostics.Process> object to get the most up-to-date main window handle if it has changed.

/// <remarks><format type="text/markdown"><![CDATA[
/// The module's entry point is the location of the function that is called during process startup, thread startup, process shutdown, and thread shutdown. While the entry point is not the address of the DllMain function, it should be close enough for most purposes.
/// > [!NOTE]
/// > Due to changes in the way that Windows loads assemblies, <xref:System.Diagnostics.ProcessModule.EntryPointAddress%2A> will always return 0 on [!INCLUDE[win8](~/includes/win8-md.md)] or [!INCLUDE[win81](~/includes/win81-md.md)] and should not be relied on for those platforms.
Copy link
Contributor

Choose a reason for hiding this comment

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

The INCLUDE files for Windows versions should be replaced.

/// a real-time process that executes for more than a very brief interval can cause
/// disk caches not to flush or cause the mouse to be unresponsive.
/// </devdoc>
/// <summary>Specifies that the process has the highest possible priority. <br />The threads of a process with <see langword="RealTime" /> priority preempt the threads of all other processes, including operating system processes performing important tasks. Thus, a <see langword="RealTime" /> priority process that executes for more than a very brief interval can cause disk caches not to flush or cause the mouse to be unresponsive.</summary>
Copy link
Contributor

Choose a reason for hiding this comment

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

Should <br /> be replaced with an actual newline?

@gewarren gewarren added the documentation Documentation bug or enhancement, does not impact product or test code label Apr 5, 2021
@jeffhandley jeffhandley marked this pull request as draft April 18, 2021 06:36
@ghost ghost closed this May 18, 2021
@ghost
Copy link

ghost commented May 18, 2021

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

@gewarren
Copy link
Contributor

gewarren commented Jun 9, 2021

I found it: <code class="lang-csharp">

@TianqiZhang This doesn't actually seem to be the correct attribute - at least it doesn't give syntax highlighting in this example. According to https://ceapex.visualstudio.com/Engineering/_workitems/edit/227459/?triage=true, it seems the proper way to specify a language is like this:

/// <code language="csharp">

@ghost ghost locked as resolved and limited conversation to collaborators Jul 10, 2021
@carlossanlop carlossanlop deleted the ProcessBackport branch February 7, 2024 06:42
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-System.Diagnostics.Process documentation Documentation bug or enhancement, does not impact product or test code
Projects
None yet
Development

Successfully merging this pull request may close these issues.

System.Diagnostics.Process: Backport MS Docs documentation to triple slash
7 participants