Conversation
WalkthroughThe pull request introduces nullability throughout the error-handling and multi-part request APIs. Test files add runtime null checks for deserialized error responses. Part.cs is refactored from a mutable class hierarchy to an immutable record with positional parameters. RestClientException members are made nullable with adjusted constructor signatures. File-scoped namespaces replace explicit namespace declarations. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 1
🧹 Nitpick comments (4)
Activout.RestClient/RestClientException.cs (3)
8-23: Consolidate the two ctors via a primary constructor to remove duplicationYou can express this exception with one primary constructor that also passes message/innerException to the base. Clearer, less code, same behavior.
Apply:
- public class RestClientException : Exception - { - public RestClientException(Uri? requestUri, HttpStatusCode statusCode, object? errorResponse) - : base(errorResponse?.ToString()) - { - RequestUri = requestUri; - StatusCode = statusCode; - ErrorResponse = errorResponse; - } - - public RestClientException(Uri? requestUri, HttpStatusCode statusCode, string? errorResponse, - Exception? innerException) - : base(errorResponse, innerException) - { - RequestUri = requestUri; - StatusCode = statusCode; - ErrorResponse = errorResponse; - } - - public Uri? RequestUri { get; } - public HttpStatusCode StatusCode { get; } - public object? ErrorResponse { get; } - ... - } + public class RestClientException(Uri? requestUri, HttpStatusCode statusCode, object? errorResponse, Exception? innerException = null) + : Exception(errorResponse?.ToString(), innerException) + { + public Uri? RequestUri { get; } = requestUri; + public HttpStatusCode StatusCode { get; } = statusCode; + public object? ErrorResponse { get; } = errorResponse; + // remaining members unchanged (see below for expression-bodied tweaks) + }As per coding guidelines.
30-33: Consider expression-bodied + a safe Try-get helper
- Minimal tweak (no behavior change): make this expression-bodied.
- Optional: add TryGetErrorResponse(out T?) that uses pattern matching to avoid InvalidCastException on mismatched T.
Apply minimal tweak:
- public T? GetErrorResponse<T>() - { - return (T?)ErrorResponse; - } + public T? GetErrorResponse<T>() => (T?)ErrorResponse;Optionally add:
public bool TryGetErrorResponse<T>(out T? result) { if (ErrorResponse is T t) { result = t; return true; } result = default; return false; }As per coding guidelines.
35-39: Prefer expression-bodied ToString for brevitySame output, cleaner form.
- public override string ToString() - { - return - $"{base.ToString()}, {nameof(RequestUri)}: {RequestUri}, {nameof(StatusCode)}: {StatusCode}, {nameof(ErrorResponse)}: {ErrorResponse}"; - } + public override string ToString() => + $"{base.ToString()}, {nameof(RequestUri)}: {RequestUri}, {nameof(StatusCode)}: {StatusCode}, {nameof(ErrorResponse)}: {ErrorResponse}";As per coding guidelines.
Activout.RestClient.Xml.Test/ErrorResponseXmlTest.cs (1)
54-56: Added NotNull guard: goodKeeps nullability honest after GetErrorResponse<T?> change. LGTM.
Optionally, use pattern matching to both assert and capture:
if (exception.GetErrorResponse<XmlErrorResponse>() is { } errorResponse) { Assert.Equal(400, errorResponse.Code); Assert.Equal("Invalid request parameter", errorResponse.Message); } else { Assert.Fail("Expected XmlErrorResponse."); }[Based on coding guidelines]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
Activout.RestClient.Test.Json/RestClientTests.cs(2 hunks)Activout.RestClient.Xml.Test/ErrorResponseXmlTest.cs(1 hunks)Activout.RestClient/Part.cs(1 hunks)Activout.RestClient/RestClientException.cs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.cs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
**/*.cs: Use [] list syntax for collections
Use file-scoped namespaces
Use primary constructors wherever possible
Use records for data transfer objects (DTOs) and immutable data structures
Use var for local variable declarations when possible
Use expression-bodied members for simple methods and properties
Use pattern matching for type checks and deconstruction
Use using statements for resource management
Use async and await for asynchronous programming
Never include "Async" in method names
Only include comments that explain why something is done, not what is done
Files:
Activout.RestClient.Test.Json/RestClientTests.csActivout.RestClient.Xml.Test/ErrorResponseXmlTest.csActivout.RestClient/RestClientException.csActivout.RestClient/Part.cs
🔇 Additional comments (3)
Activout.RestClient/RestClientException.cs (2)
4-4: File-scoped namespace: good moveMatches the guideline and reduces nesting noise. [Based on coding guidelines]
25-29: Property nullability looks correctPublic surface now accurately reflects possible nulls. LGTM.
Activout.RestClient.Test.Json/RestClientTests.cs (1)
115-116: Non-null asserts after GetErrorResponse<T?>: goodMatches new nullable return, prevents accidental null deref in tests. LGTM.
Also applies to: 169-170
1 participant
Summary by CodeRabbit
Release Notes
Tests
Refactor