doc(AttributeTable): support base type parameter#7554
Merged
Conversation
Contributor
Reviewer's GuideRefines component XML documentation for display/validation components and enhances the AttributeTable documentation resolution to support base-type/property-type XML doc lookups, along with a small styling adjustment for attribute markers. Class diagram for ComponentAttributeCacheService documentation resolutionclassDiagram
class ComponentAttributeCacheService {
- static ConcurrentDictionary<Type, List<AttributeItem>> Cache
+ static List<AttributeItem> GetAttributes(Type type)
- static List<AttributeItem> GetAttributeCore(Type type)
- static string GetPropertySummary(XmlDocument xmlDoc, PropertyInfo property)
- static XElement FindSummaryElement(XmlDocument xmlDoc, string memberName)
- static string GetLocalizedSummary(XElement summaryElement)
}
class AttributeItem {
+ string Name
+ string Description
+ string Type
+ string DefaultValue
}
ComponentAttributeCacheService --> AttributeItem : creates
Flow diagram for updated XML doc lookup in ComponentAttributeCacheServiceflowchart TD
A[Start: resolve property XML doc] --> B[Get property.DeclaringType]
B --> C{DeclaringType is null?}
C -- No --> D[Set type = property.DeclaringType]
C -- Yes --> E[Set type = property.PropertyType]
D --> F[Get type.FullName]
E --> F
F --> G{type.FullName is null?}
G -- No --> H[Set typeName = type.FullName]
G -- Yes --> I[Set typeName = BootstrapBlazor.Components. + type.Name]
H --> J[Build memberName = P: + typeName + . + property.Name]
I --> J
J --> K[FindSummaryElement xmlDoc memberName]
K --> L{summaryElement is null?}
L -- Yes --> M[Return null]
L -- No --> N[Return GetLocalizedSummary summaryElement]
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The new XML-doc lookup in
ComponentAttributeCacheService.GetPropertySummaryusesproperty.DeclaringType ?? property.PropertyTypeand then guesses a namespace whenFullNameis null; this can produce incorrect member names (properties are defined on the declaring type, not the value type) and brittle lookups—consider defaulting only toDeclaringTypeand handlingFullName == nullmore conservatively (e.g., skipping or logging). - Replacing explicit bilingual summaries on overriding lifecycle methods in
DisplayBasewith<inheritdoc/>may drop the localized XML docs if the base methods are not similarly documented; verify that consumers still get the intended Chinese/English descriptions or keep the explicit summaries where needed.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new XML-doc lookup in `ComponentAttributeCacheService.GetPropertySummary` uses `property.DeclaringType ?? property.PropertyType` and then guesses a namespace when `FullName` is null; this can produce incorrect member names (properties are defined on the declaring type, not the value type) and brittle lookups—consider defaulting only to `DeclaringType` and handling `FullName == null` more conservatively (e.g., skipping or logging).
- Replacing explicit bilingual summaries on overriding lifecycle methods in `DisplayBase` with `<inheritdoc/>` may drop the localized XML docs if the base methods are not similarly documented; verify that consumers still get the intended Chinese/English descriptions or keep the explicit summaries where needed.
## Individual Comments
### Comment 1
<location> `src/BootstrapBlazor.Server/Services/ComponentAttributeCacheService.cs:63-64` </location>
<code_context>
if (xmlDoc == null) return null;
- var memberName = $"P:{property.DeclaringType?.FullName}.{property.Name}";
+ var type = property.DeclaringType ?? property.PropertyType;
+ var typeName = type.FullName ?? $"BootstrapBlazor.Components.{type.Name}";
+ var memberName = $"P:{typeName}.{property.Name}";
</code_context>
<issue_to_address>
**suggestion:** Avoid shadowing the method parameter `type` with a local variable of the same name.
This shadows the `GetAttributeCore` parameter and hurts readability. Please rename the local (e.g., `declaringTypeOrPropertyType`) to make its purpose clear and avoid confusion with the outer `type` parameter.
```suggestion
var declaringTypeOrPropertyType = property.DeclaringType ?? property.PropertyType;
var typeName = declaringTypeOrPropertyType.FullName ?? $"BootstrapBlazor.Components.{declaringTypeOrPropertyType.Name}";
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Contributor
There was a problem hiding this comment.
Pull request overview
This pull request aims to improve the AttributeTable component's ability to handle base type parameters when generating documentation. The changes include documentation improvements, XML documentation lookup logic modifications, and build configuration updates.
Changes:
- Enhanced XML documentation member name resolution to handle properties from base types
- Improved Chinese documentation quality with spelling corrections and better formatting
- Modified build targets to ensure XML documentation files are properly copied
- Minor styling adjustments to the attribute table markers
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/BootstrapBlazor/Components/Validate/ValidateBase.cs | Updated XML documentation comments to be more accurate (changed "获得/设置" to "获得" for read-only property ValidCss, and added "获得/设置" for Required property) |
| src/BootstrapBlazor/Components/Display/DisplayBase.cs | Improved Chinese documentation with spelling corrections (裁减→裁剪), removed extra spaces, added proper <see cref> tags, and replaced method documentation with <inheritdoc/> |
| src/BootstrapBlazor.Server/Services/ComponentAttributeCacheService.cs | Modified GetSummary method to support base type parameters by adding fallback logic for type resolution |
| src/BootstrapBlazor.Server/Directory.Build.targets | Removed file existence condition and skip unchanged files optimization from XML copy task |
| src/BootstrapBlazor.Server/Components/Components/AttributeTable.razor.css | Adjusted marker styling (font-size: 85%, added color) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (xmlDoc == null) return null; | ||
|
|
||
| var memberName = $"P:{property.DeclaringType?.FullName}.{property.Name}"; | ||
| var type = property.DeclaringType ?? property.PropertyType; |
There was a problem hiding this comment.
The fallback logic when property.DeclaringType is null appears incorrect. Using property.PropertyType as a fallback doesn't make semantic sense - the declaring type (which class the property is declared in) and the property type (the type of the value) are completely different concepts.
For XML documentation lookup, we need the type where the property is declared, not the type of the property's value. If DeclaringType is null, this would typically indicate a reflection issue, and falling back to PropertyType would likely result in looking up documentation in the wrong location.
Consider either:
- Removing the fallback to
PropertyTypeif it's not a valid scenario - If this is intended to handle a specific edge case, add a comment explaining when and why
DeclaringTypewould be null and whyPropertyTypeis the correct fallback
Suggested change
| var type = property.DeclaringType ?? property.PropertyType; | |
| // DeclaringType is required to build the correct XML documentation member name. | |
| // If it is null, we cannot reliably locate the summary, so return null instead | |
| // of falling back to PropertyType (which represents the value type, not the owner). | |
| var type = property.DeclaringType; | |
| if (type == null) | |
| { | |
| return null; | |
| } |
|
|
||
| var memberName = $"P:{property.DeclaringType?.FullName}.{property.Name}"; | ||
| var type = property.DeclaringType ?? property.PropertyType; | ||
| var typeName = type.FullName ?? $"BootstrapBlazor.Components.{type.Name}"; |
There was a problem hiding this comment.
The fallback construction $"BootstrapBlazor.Components.{type.Name}" when type.FullName is null may not produce accurate member names for XML documentation lookup. This assumes all types without a FullName belong to the BootstrapBlazor.Components namespace, which may not always be true.
FullName can be null for generic type parameters and some special types. For generic types, you might want to handle them differently. Consider adding validation or a comment explaining which types are expected to have null FullName and why this fallback is appropriate for those specific cases.
Suggested change
| var typeName = type.FullName ?? $"BootstrapBlazor.Components.{type.Name}"; | |
| var typeName = type.FullName; | |
| // FullName can be null for generic type parameters and some special types. | |
| // In such cases we skip XML documentation lookup instead of fabricating a namespace, | |
| // as that may not produce an accurate member name. | |
| if (string.IsNullOrEmpty(typeName)) | |
| { | |
| return null; | |
| } |
| @@ -185,7 +183,6 @@ protected override void OnParametersSet() | |||
| /// <para lang="en">Format Value to String Method</para> | |||
| /// </summary> | |||
| /// <param name="value">The value to format.</param> | |||
There was a problem hiding this comment.
The removal of the <returns> tag from the XML documentation is inconsistent with standard C# documentation practices. Methods should document their return values, especially for public or protected virtual methods. The English comment "A string representation of the value" provided useful information about what the method returns.
Consider keeping the <returns> tag to maintain complete documentation.
Suggested change
| /// <param name="value">The value to format.</param> | |
| /// <param name="value">The value to format.</param> | |
| /// <returns> | |
| /// <para lang="zh">值的字符串表示形式。</para> | |
| /// <para lang="en">A string representation of the value.</para> | |
| /// </returns> |
| </ItemGroup> | ||
| <Message Text="Copying XML docs to output path: $(OutputPath)" Importance="High" /> | ||
| <Copy SourceFiles="@(XmlDocsToCopy)" DestinationFolder="$(OutputPath)" SkipUnchangedFiles="true" Condition="Exists('%(RootDir)%(Directory)%(Filename)%(Extension)')" /> | ||
| <Copy SourceFiles="@(XmlDocsToCopy)" DestinationFolder="$(OutputPath)" /> |
There was a problem hiding this comment.
The removal of SkipUnchangedFiles="true" and the Condition attribute from the Copy task may cause unnecessary file copying on every build, potentially slowing down build times. The original attributes were performance optimizations:
SkipUnchangedFiles="true"avoids copying files that haven't changed- The
Conditionprevents errors when files don't exist
Unless there's a specific reason these optimizations are causing issues (such as stale documentation not being updated), consider keeping them. If they must be removed to fix the issue mentioned in #7553, please add a comment explaining why.
Suggested change
| <Copy SourceFiles="@(XmlDocsToCopy)" DestinationFolder="$(OutputPath)" /> | |
| <Copy SourceFiles="@(XmlDocsToCopy)" DestinationFolder="$(OutputPath)" SkipUnchangedFiles="true" Condition="Exists('%(RootDir)%(Directory)%(Filename)%(Extension)')" /> |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7554 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 748 748
Lines 33000 33000
Branches 4588 4588
=========================================
Hits 33000 33000
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Link issues
fixes #7553
Summary By Copilot
Regression?
Risk
Verification
Packaging changes reviewed?
☑️ Self Check before Merge
Summary by Sourcery
Improve component metadata display and documentation for attribute tables and validation components.
Enhancements: