Doc updates#25
Conversation
WalkthroughThis pull request updates XML documentation comments across logging and validation modules, improving clarity and consistency. Several files receive behavioral changes, including added validation logic, relaxed constraints, and a property becoming writable, while maintaining backward compatibility with existing public APIs. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/Logging.AzureAppServices/src/BatchingLoggerOptions.cs (1)
33-53: Error message is inconsistent with the validation logic.The check
value <= 0rejects both zero and negative values, but the error message states "must be non-negative." Zero is non-negative, so the message is misleading. Since a queue size of 0 doesn't make practical sense, the validation is correct but the message should say "must be positive."Suggested fix
if (value <= 0) { - throw new ArgumentOutOfRangeException(nameof(value), $"{nameof(BackgroundQueueSize)} must be non-negative."); + throw new ArgumentOutOfRangeException(nameof(value), $"{nameof(BackgroundQueueSize)} must be positive."); }src/Logging.AzureAppServices/src/AzureFileLoggerOptions.cs (1)
42-58: Documentation inconsistent with validation logic.The validation on line 53 was changed from
value <= 0tovalue < 0, which now allows0as a valid value. However:
- Line 42 states "strictly positive value" - but 0 is not strictly positive
- Line 55 error message says "must be positive" - but 0 would pass validation
Either the validation should reject 0 (use
value <= 0), or the documentation and error message should be updated to reflect that 0 is allowed (e.g., "non-negative value").Option A: If 0 should NOT be allowed (restore original behavior)
- if (value < 0) + if (value <= 0)Option B: If 0 IS intentionally allowed (update docs and error message)
/// <summary> - /// Gets or sets a strictly positive value representing the maximum retained file count. + /// Gets or sets a non-negative value representing the maximum retained file count, or null for no limit. /// </summary>- throw new ArgumentOutOfRangeException(nameof(value), $"{nameof(RetainedFileCountLimit)} must be positive."); + throw new ArgumentOutOfRangeException(nameof(value), $"{nameof(RetainedFileCountLimit)} must be non-negative.");
🧹 Nitpick comments (1)
src/Validation/src/ValidationOptions.cs (1)
66-66: Minor style inconsistency: opening brace placement.The opening brace is on the same line as the method signature here, but
TryGetValidatableTypeInfo(line 44-45) uses a newline before the brace. Consider aligning the style for consistency.Suggested fix
- public bool TryGetValidatableParameterInfo(ParameterInfo parameterInfo, [NotNullWhen(true)] out IValidatableInfo? validatableInfo) { + public bool TryGetValidatableParameterInfo(ParameterInfo parameterInfo, [NotNullWhen(true)] out IValidatableInfo? validatableInfo) + {
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
src/Caching/SqlServer/src/SqlServerCacheOptions.cssrc/HttpClientFactory/Polly/src/DependencyInjection/PollyHttpClientBuilderExtensions.cssrc/Logging.AzureAppServices/src/AzureAppServicesLoggerFactoryExtensions.cssrc/Logging.AzureAppServices/src/AzureBlobLoggerContext.cssrc/Logging.AzureAppServices/src/AzureBlobLoggerOptions.cssrc/Logging.AzureAppServices/src/AzureFileLoggerOptions.cssrc/Logging.AzureAppServices/src/BatchingLoggerOptions.cssrc/Logging.AzureAppServices/src/BatchingLoggerProvider.cssrc/Logging.AzureAppServices/src/FileLoggerProvider.cssrc/Validation/src/IValidatableInfoResolver.cssrc/Validation/src/SkipValidationAttribute.cssrc/Validation/src/ValidateContext.cssrc/Validation/src/ValidationOptions.cs
🧰 Additional context used
🧬 Code graph analysis (3)
src/Validation/src/ValidationOptions.cs (3)
src/Validation/src/SkipValidationAttribute.cs (1)
Experimental(17-21)src/Validation/src/IValidatableInfoResolver.cs (2)
TryGetValidatableTypeInfo(24-24)TryGetValidatableParameterInfo(32-32)src/Validation/src/RuntimeValidatableParameterInfoResolver.cs (2)
TryGetValidatableTypeInfo(18-22)TryGetValidatableParameterInfo(24-58)
src/Logging.AzureAppServices/src/AzureBlobLoggerOptions.cs (1)
src/Logging.AzureAppServices/src/BatchingLoggerOptions.cs (1)
BatchingLoggerOptions(11-86)
src/Validation/src/IValidatableInfoResolver.cs (2)
src/Validation/test/Microsoft.Extensions.Validation.Tests/ValidatableTypeInfoTests.cs (1)
TryGetValidatableTypeInfo(897-906)src/Validation/src/RuntimeValidatableParameterInfoResolver.cs (1)
TryGetValidatableTypeInfo(18-22)
🔇 Additional comments (30)
src/Caching/SqlServer/src/SqlServerCacheOptions.cs (1)
10-49: Docs look consistent and clearer.The updated summaries and added
<value>tags improve clarity without altering behavior.src/Logging.AzureAppServices/src/AzureBlobLoggerContext.cs (5)
8-10: Doc summary wording is clear and precise. Line 9 reads well and aligns with the type’s intent.
13-15: Constructor summary wording is consistent. Line 14’s “Creates” phrasing matches standard doc style.
26-29: Property doc clarity improved. Line 27’s “Gets” phrasing is consistent with the read-only property.
31-34: Identifier doc is clearer. Line 32 succinctly explains the value format and references.
36-39: Timestamp doc is concise and accurate. Line 37 reads well and matches semantics.src/Logging.AzureAppServices/src/AzureAppServicesLoggerFactoryExtensions.cs (2)
15-17: LGTM!The class summary accurately describes the purpose of this extension class.
20-25: LGTM!The parameter documentation is consistent with the overloaded method at line 36.
src/Logging.AzureAppServices/src/BatchingLoggerProvider.cs (1)
13-15: Documentation improvements look good.The updated XML documentation for the class summary,
IsEnabledproperty, andIntervalAsyncmethod provides clearer and more consistent descriptions.Also applies to: 61-64, 126-132
src/HttpClientFactory/Polly/src/DependencyInjection/PollyHttpClientBuilderExtensions.cs (1)
13-16: Documentation updates improve consistency across method overloads.The rephrased summaries using "will surround request execution with" provide a more uniform description pattern. The grammar refinement in the remarks section is also an improvement.
Also applies to: 19-21, 47-49, 79-81, 114-116, 151-153, 187-213, 238-241
src/Logging.AzureAppServices/src/FileLoggerProvider.cs (1)
14-16: LGTM!The documentation wording changes align with the consistent style applied across the other logging providers in this PR.
Also applies to: 27-28
src/Logging.AzureAppServices/src/AzureBlobLoggerOptions.cs (3)
9-11: Documentation improvement looks good.
22-30: Good addition of input validation forBlobName.The validation ensures the blob name cannot be set to null or empty, preventing potential issues during blob operations. This is consistent with the validation patterns used in the parent
BatchingLoggerOptionsclass.
32-42: Documentation enhancement with<value>tag is helpful.The added
<value>tag clearly documents the default format, which aids discoverability for API consumers.src/Logging.AzureAppServices/src/BatchingLoggerOptions.cs (2)
8-10: Documentation improvement looks good.
55-72: Documentation enhancements look good.The added
<value>tags forBatchSizeandIncludeScopesproperties improve API documentation consistency.Also applies to: 74-85
src/Validation/src/IValidatableInfoResolver.cs (1)
16-24: Documentation improvements follow standard Try-pattern conventions.The updated parameter and return descriptions align with .NET documentation conventions for
TryGet*methods and match the behavior shown in the implementations (e.g.,RuntimeValidatableParameterInfoResolver).Also applies to: 26-32
src/Validation/src/ValidationOptions.cs (4)
10-10: LGTM!Documentation improvements are clear and accurate - the class summary is more precise and the resolver behavior is well-documented.
Also applies to: 16-16
28-33: LGTM!Good addition of
<value>and<remarks>tags. The documentation now clearly explains the default value and the purpose of the depth limit.
41-46: LGTM!Good improvements:
- Using
<see langword="null" />is the proper XML documentation convention- Initializing
validatableTypeInfotonullat the method start ensures the out parameter is always assigned, satisfying the contract even if the loop doesn't execute
63-64: LGTM!Consistent use of
<see langword="null" />matching the pattern inTryGetValidatableTypeInfo.src/Validation/src/SkipValidationAttribute.cs (1)
9-16: LGTM!Documentation is well-structured with a concise summary and detailed remarks explaining the attribute's behavior for each target type. The remarks accurately match the
AttributeUsagedeclaration.src/Validation/src/ValidateContext.cs (4)
26-26: LGTM!Adding
language="csharp"to the code block enables proper syntax highlighting in documentation tools.
41-43: LGTM!The added remarks clearly explain the prefix usage pattern with a helpful example.
54-58: LGTM!Good restructuring - the remarks now clearly document the dictionary's key/value semantics and the lazy initialization behavior.
64-66: LGTM!The remarks appropriately document the purpose of depth tracking for preventing stack overflows.
src/Logging.AzureAppServices/src/AzureFileLoggerOptions.cs (4)
10-10: LGTM!Clear and concise class summary.
18-27: LGTM!Well-structured documentation with proper
<value>and<remarks>tags. The behavior when the log is full is now clearly documented.
61-68: LGTM!Clear documentation explaining the file naming convention with the date suffix format.
80-80: LGTM!Internal property for log directory path - appropriate for internal configuration.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
2 participants
Benchmark PR from qodo-benchmark#96
Summary by CodeRabbit
Documentation
Bug Fixes
New Features
✏️ Tip: You can customize this high-level summary in your review settings.