-
Notifications
You must be signed in to change notification settings - Fork 5.3k
Add XML documentation to DynamicMethod from dotnet-api-docs #124227
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
base: main
Are you sure you want to change the base?
Conversation
Co-authored-by: gewarren <24882762+gewarren@users.noreply.github.com>
gewarren
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot Please address all feedback.
| /// <exception cref="InvalidOperationException">The dynamic method has no method body.</exception> | ||
| /// <exception cref="ArgumentException"><paramref name="delegateType" /> has the wrong number of parameters or the wrong parameter types.</exception> | ||
| /// <remarks> | ||
| /// For more information about this API, see <see href="https://raw.githubusercontent.com/dotnet/docs/main/docs/fundamentals/runtime-libraries/system-reflection-emit-dynamicmethod.md">Supplemental API remarks for DynamicMethod</see>. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot The linked PR is adding multiple new articles. Each member should link to their specific article. For example, this one should be:
| /// For more information about this API, see <see href="https://raw.githubusercontent.com/dotnet/docs/main/docs/fundamentals/runtime-libraries/system-reflection-emit-dynamicmethod.md">Supplemental API remarks for DynamicMethod</see>. | |
| /// For more information about this API, see <see href="https://raw.githubusercontent.com/dotnet/docs/main/docs/fundamentals/runtime-libraries/system-reflection-emit-dynamicmethod-createdelegate.md">Supplemental API remarks for CreateDelegate</see>. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in commit 2f4fe14. Updated to use the specific CreateDelegate article link.
| /// Returns a <see cref="DynamicILInfo" /> object that can be used to generate a method body from metadata tokens, scopes, and Microsoft intermediate language (MSIL) streams. | ||
| /// </summary> | ||
| /// <returns>A <see cref="DynamicILInfo" /> object that can be used to generate a method body from metadata tokens, scopes, and MSIL streams.</returns> | ||
| public DynamicILInfo GetDynamicILInfo() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot Why didn't you backport the <remarks> here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added GetDynamicILInfo remarks in commit 2f4fe14.
| /// <summary> | ||
| /// Gets the name of the dynamic method. | ||
| /// </summary> | ||
| /// <value>The name of the dynamic method.</value> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing remarks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added Name property remarks in commit 2f4fe14.
| /// <summary> | ||
| /// Gets the type that declares the dynamic method. | ||
| /// </summary> | ||
| /// <value>Always <see langword="null" /> for dynamic methods.</value> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing remarks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added DeclaringType property remarks in commit 2f4fe14.
| /// <summary> | ||
| /// Gets the class object that was used to obtain the instance of the dynamic method. | ||
| /// </summary> | ||
| /// <value>Always <see langword="null" /> for dynamic methods.</value> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing remarks. Since these remarks include a code example, that should go in an <example> tag, not the <remarks> tag. Also extract the code from the linked file in the dotnet/dotnet-api-docs repo and put it inline here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added ReflectedType property remarks in commit 2f4fe14.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot I still don't see the examples from the remarks. They should be inserted into the /// similar to this example code:
runtime/src/libraries/Microsoft.Extensions.Logging.Abstractions/src/LoggerExtensions.cs
Line 26 in b8e7226
| /// <code language="csharp"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added example sections with code snippet references in commit ac08171. Examples added to all 12 members that have them in dotnet-api-docs: Name, DeclaringType, ReflectedType, Module, Attributes, CallingConvention, GetParameters, ReturnType, ReturnTypeCustomAttributes, DefineParameter, ToString, and InitLocals.
| /// Gets the return type of the dynamic method. | ||
| /// </summary> | ||
| /// <value>A <see cref="Type" /> representing the return type of the dynamic method; or <see cref="void" /> if the method has no return type.</value> | ||
| public override Type ReturnType => _returnType; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing remarks here and for ReturnParameter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added ReturnParameter and ReturnTypeCustomAttributes remarks in commit 2f4fe14.
| /// Gets the custom attributes of the return type for the dynamic method. | ||
| /// </summary> | ||
| /// <value>An <see cref="ICustomAttributeProvider" /> representing the custom attributes of the return type for the dynamic method.</value> | ||
| public override ICustomAttributeProvider ReturnTypeCustomAttributes => new EmptyCAHolder(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing remarks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added DefineParameter method remarks in commit 2f4fe14.
| /// <paramref name="position" /> is less than 0. | ||
| /// -or- | ||
| /// <paramref name="position" /> is greater than the number of parameters of the dynamic method.</exception> | ||
| public ParameterBuilder? DefineParameter(int position, ParameterAttributes attributes, string? parameterName) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing remarks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added ToString method remarks in commit 2f4fe14.
| /// Gets or sets a value indicating whether the local variables in the method are zero-initialized. | ||
| /// </summary> | ||
| /// <value><see langword="true" /> if the local variables in the method are zero-initialized; otherwise, <see langword="false" />. The default is <see langword="true" />.</value> | ||
| public bool InitLocals |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing remarks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added InitLocals property remarks in commit 2f4fe14.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since the /// docs are already in the DynamicMethod.CoreCLR.cs file, I don't think we need to add them here too.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed duplicate documentation from Mono file in commit 2f4fe14. Documentation is now only in CoreCLR file.
Co-authored-by: gewarren <24882762+gewarren@users.noreply.github.com>
All feedback addressed in commit 2f4fe14. Changes include:
|
Co-authored-by: gewarren <24882762+gewarren@users.noreply.github.com>
Co-authored-by: gewarren <24882762+gewarren@users.noreply.github.com>
| /// Returns a string representation of the dynamic method. | ||
| /// </summary> | ||
| /// <returns>A string representation of the dynamic method, showing the return type, name, and parameter types.</returns> | ||
| /// <example> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot Don't include the Visual Basic examples. And if the C# examples are linked from a separate file, please pull the code in to be inline. In this case, the file you need to get the code from is https://github.com/dotnet/dotnet-api-docs/blob/main/snippets/csharp/System.Reflection.Emit/DynamicMethod/Overview/source.cs.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in commit 488b3c0. Converted all examples to inline C# code and removed Visual Basic references. Code pulled from the dotnet-api-docs snippet file for all 8 members with examples.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@copilot Sorry, upon further discussion, we do want the code examples in a separate .cs file. Please use #region syntax similar to the Newtonsoft XML docs: https://github.com/JamesNK/Newtonsoft.Json/blob/e5f67150a0a2cd6fafcee483add00c1bb0fae306/Src/Newtonsoft.Json/Linq/JObject.cs#L54. Place the files in a location that makes sense to you.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in commit 7802100. Created DynamicMethod.Examples.cs with all code examples in #region blocks, following the Newtonsoft.Json pattern. All 12 members now reference the separate file using <code lang="cs" source="DynamicMethod.Examples.cs" region="..." />.
Co-authored-by: gewarren <24882762+gewarren@users.noreply.github.com>
Co-authored-by: gewarren <24882762+gewarren@users.noreply.github.com>
Contributes to dotnet/dotnet-api-docs#12307.
Description
Backported XML documentation comments from
dotnet/dotnet-api-docsto theDynamicMethodclass. Per requirements, specific members (constructors,CreateDelegate,GetILGenerator,Invoke,IsSecurityCritical) include remarks linking to supplemental API documentation. Each member links to its specific article in the dotnet/docs repository.These documents will become available when dotnet/docs#51571 merges.
Changes Made
Documentation Added
<summary>and<remarks>with supplemental link to theDynamicMethodclass declaration<summary>,<param>,<exception>, and<remarks>with supplemental links to the parent type articleCode Examples
#regionblocks, following the Newtonsoft.Json pattern<example>sections to 12 members (Name, DeclaringType, ReflectedType, Module, Attributes, CallingConvention, GetParameters, ReturnType, ReturnTypeCustomAttributes, DefineParameter, ToString, InitLocals) that reference the Examples.cs file using<code lang="cs" source="DynamicMethod.Examples.cs" region="..." />Other Changes
Files Modified
src/libraries/System.Private.CoreLib/src/System/Reflection/Emit/DynamicMethod.cs- Main file with complete documentation including all remarks and example references (395 lines added)src/libraries/System.Private.CoreLib/src/System/Reflection/Emit/DynamicMethod.Examples.cs- New file with code examples in #region blocks (143 lines added)src/coreclr/System.Private.CoreLib/src/System/Reflection/Emit/DynamicMethod.CoreCLR.cs- CoreCLR-specific members with fixed supplemental links (59 lines added, 11 lines modified)src/mono/System.Private.CoreLib/src/System/Reflection/Emit/DynamicMethod.Mono.cs- Removed duplicate documentation (48 lines removed)Total: 549 net lines added across all implementations with complete remarks and examples backported from dotnet-api-docs.
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.