Make RestClientContext a record and move logic to RestClientBuilder#129
Make RestClientContext a record and move logic to RestClientBuilder#129
record and move logic to RestClientBuilder#129Conversation
|
Note Currently processing new changes in this PR. This may take a few minutes, please wait... 📒 Files selected for processing (1)
WalkthroughReworks RestClient into a non-generic dynamic client, centralizes builder state and attribute handling, makes RestClientContext an immutable record, exposes StringSerializer singleton, tightens serializer/deserializer media-type handling (System.Text.Json & Newtonsoft), adds DummyRequestLogger, and narrows serializer/deserializer Order accessors. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Caller (dynamic call)
participant Rest as RestClient
participant Binder as InvokeMemberBinder
participant Cache as _requestHandlers
participant Factory as RequestHandler (factory)
participant Handler as RequestHandler.Send
Client->>Rest: dynamicMethod(...)
Rest->>Binder: TryInvokeMember(binder, args)
Rest->>Cache: lookup binder.Name
alt handler cached
Cache-->>Rest: cached handler
Rest->>Handler: cached.Send(args)
else not cached
Rest->>Factory: new RequestHandler(binder,...)
Factory->>Cache: cache handler
rect rgb(220,245,220)
Rest->>Handler: new.Send(args)
end
end
Handler-->>Client: result / Task
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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: 3
🧹 Nitpick comments (14)
Activout.RestClient/Serialization/Implementation/StringSerializer.cs (2)
12-15: Case-insensitive match and expression-bodied members.Media types are case-insensitive. Also these methods are perfect candidates for expression bodies.
Apply:
- public HttpContent Serialize(object? data, Encoding encoding, MediaType mediaType) - { - return new StringContent(data?.ToString() ?? "", encoding, mediaType.Value); - } + public HttpContent Serialize(object? data, Encoding encoding, MediaType mediaType) + => new StringContent(data?.ToString() ?? "", encoding, mediaType.Value); @@ - public bool CanSerialize(MediaType mediaType) - { - return mediaType.Value.StartsWith("text/"); - } + public bool CanSerialize(MediaType mediaType) + => mediaType.Value.StartsWith("text/", StringComparison.OrdinalIgnoreCase);As per coding guidelines.
Also applies to: 17-20
6-6: Consider sealing the type.If inheritance isn’t required, seal for clarity and minor JIT optimizations.
-public class StringSerializer : ISerializer +public sealed class StringSerializer : ISerializerActivout.RestClient/Implementation/DummyRequestLogger.cs (1)
10-18: Use expression-bodied members.Tiny no-op logger; simplify.
- public IDisposable TimeOperation(HttpRequestMessage httpRequestMessage) - { - return this; - } + public IDisposable TimeOperation(HttpRequestMessage httpRequestMessage) => this; @@ - public void Dispose() - { - // Do nothing - } + public void Dispose() { /* no-op */ }As per coding guidelines.
Activout.RestClient.Newtonsoft.Json/RestClientBuilderNewtonsoftJsonExtensions.cs (1)
11-17: Avoid First() when media type list could be empty.Be defensive; set Accept regardless, set Content-Type only when a first exists.
- var settings = jsonSerializerSettings ?? DefaultJsonSerializerSettings; + var settings = jsonSerializerSettings ?? DefaultJsonSerializerSettings; @@ - builder.Accept(string.Join(", ", SupportedMediaTypes.Select(type => type.Value))); - builder.ContentType(SupportedMediaTypes.First()); + var accept = string.Join(", ", SupportedMediaTypes.Select(type => type.Value)); + builder.Accept(accept); + var first = SupportedMediaTypes.FirstOrDefault(); + if (first is not null) + { + builder.ContentType(first); + }Activout.RestClient.Json/RestClientBuilderJsonExtensions.cs (1)
22-27: Guard against empty media types.Avoid First() on an empty array; fail fast or skip setting Content-Type.
- mediaTypes ??= MediaTypes; + mediaTypes ??= MediaTypes; + if (mediaTypes.Length == 0) + throw new ArgumentException("At least one media type is required.", nameof(mediaTypes)); @@ - builder.Accept(string.Join(", ", mediaTypes.Select(type => type.Value))); - builder.ContentType(mediaTypes.First()); + builder.Accept(string.Join(", ", mediaTypes.Select(type => type.Value))); + builder.ContentType(mediaTypes[0]);As per coding guidelines.
Activout.RestClient/Implementation/RestClientBuilder.cs (4)
35-35: Use collection expressions for empty list initialization.Modern C#: prefer [] over new List().
- private List<KeyValuePair<string, object>> _defaultHeaders = new List<KeyValuePair<string, object>>(); + private List<KeyValuePair<string, object>> _defaultHeaders = [];As per coding guidelines.
52-56: Avoid duplicate default headers.Use AddOrReplace to prevent repeated entries for the same header.
- _defaultHeaders.Add(new KeyValuePair<string, object>(name, value)); + _defaultHeaders.AddOrReplaceHeader(name, value, replace: true);Based on helpers in Activout.RestClient.Implementation.HeaderListExtensions.
174-184: Minor: tighten AddTrailingSlash.Use char EndsWith and string interpolation for small clarity/perf wins.
- private static Uri AddTrailingSlash(Uri apiUri) + private static Uri AddTrailingSlash(Uri apiUri) { var uriBuilder = new UriBuilder(apiUri ?? throw new ArgumentNullException(nameof(apiUri))); - if (uriBuilder.Path.EndsWith("/")) + if (uriBuilder.Path.EndsWith('/')) { return apiUri; } - uriBuilder.Path = uriBuilder.Path + "/"; + uriBuilder.Path = $"{uriBuilder.Path}/"; return uriBuilder.Uri; }As per coding guidelines.
151-172: Consider inheritable attributes (optional).If attributes can be declared on base interfaces/types, use GetCustomAttributes(inherit: true).
- var attributes = type.GetCustomAttributes(); + var attributes = type.GetCustomAttributes(inherit: true);Please confirm whether attribute inheritance is desired for your API surface.
Activout.RestClient/Implementation/RestClientContext.cs (2)
12-26: DefaultHeaders should be immutable at the API surfaceThis record models immutable config, but
DefaultHeadersisList<...>, which is mutable. Expose it asIReadOnlyList<...>to preserve immutability and thread-safety while keeping current usages (e.g., AddRange) intact.As per coding guidelines.
- List<KeyValuePair<string, object>> DefaultHeaders, + IReadOnlyList<KeyValuePair<string, object>> DefaultHeaders,
28-28: Use expression-bodied property for the simple computed memberTiny polish to align with the guideline on expression-bodied members.
As per coding guidelines.
- public bool UseDomainException { get; } = DomainExceptionType != null; + public bool UseDomainException => DomainExceptionType != null;Activout.RestClient/Implementation/RestClient.cs (3)
15-16: Use GetOrAdd to avoid duplicate handler creation under concurrencyYou already use
ConcurrentDictionary; switching the field type and usingGetOrAddremoves the create-then-assign race and simplifies code.- private readonly IDictionary<MethodInfo, RequestHandler> _requestHandlers = - new ConcurrentDictionary<MethodInfo, RequestHandler>(); + private readonly ConcurrentDictionary<MethodInfo, RequestHandler> _requestHandlers = new();- if (!_requestHandlers.TryGetValue(method, out var requestHandler)) - { - requestHandler = new RequestHandler(method, _context); - _requestHandlers[method] = requestHandler; - } + var requestHandler = _requestHandlers.GetOrAdd(method, m => new RequestHandler(m, _context));Also applies to: 38-42
24-27: Minor tidy: expression-bodied + distinct namesKeeps it concise and avoids duplicate member names for better dynamic tooling hints.
As per coding guidelines.
- public override IEnumerable<string> GetDynamicMemberNames() - { - return _type.GetMembers().Select(x => x.Name); - } + public override IEnumerable<string> GetDynamicMemberNames() => + _type.GetMembers().Select(x => x.Name).Distinct();
10-22: Guardrails (optional): validate constructor inputsIf
typeorcontextis ever null, later reflection/dispatch will NRE. Consider simple guards to fail fast.- public RestClient(Type type, RestClientContext context) + public RestClient(Type type, RestClientContext context) { - _type = type; - _context = context; + _type = type ?? throw new ArgumentNullException(nameof(type)); + _context = context ?? throw new ArgumentNullException(nameof(context)); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
Activout.RestClient.Json/RestClientBuilderJsonExtensions.cs(2 hunks)Activout.RestClient.Newtonsoft.Json.Test/RestClientTests.cs(1 hunks)Activout.RestClient.Newtonsoft.Json/RestClientBuilderNewtonsoftJsonExtensions.cs(2 hunks)Activout.RestClient/Implementation/DummyRequestLogger.cs(1 hunks)Activout.RestClient/Implementation/RestClient.cs(1 hunks)Activout.RestClient/Implementation/RestClientBuilder.cs(2 hunks)Activout.RestClient/Implementation/RestClientContext.cs(1 hunks)Activout.RestClient/Serialization/Implementation/StringSerializer.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/Implementation/DummyRequestLogger.csActivout.RestClient.Newtonsoft.Json.Test/RestClientTests.csActivout.RestClient.Json/RestClientBuilderJsonExtensions.csActivout.RestClient.Newtonsoft.Json/RestClientBuilderNewtonsoftJsonExtensions.csActivout.RestClient/Implementation/RestClient.csActivout.RestClient/Implementation/RestClientBuilder.csActivout.RestClient/Implementation/RestClientContext.csActivout.RestClient/Serialization/Implementation/StringSerializer.cs
🧬 Code graph analysis (6)
Activout.RestClient.Json/RestClientBuilderJsonExtensions.cs (5)
Activout.RestClient.Json/SystemTextJsonDefaults.cs (1)
SystemTextJsonDefaults(43-49)Activout.RestClient.Newtonsoft.Json/RestClientBuilderNewtonsoftJsonExtensions.cs (1)
IRestClientBuilder(8-19)Activout.RestClient/Implementation/RestClientBuilder.cs (13)
IRestClientBuilder(40-44)IRestClientBuilder(46-50)IRestClientBuilder(52-56)IRestClientBuilder(58-62)IRestClientBuilder(64-68)IRestClientBuilder(70-74)IRestClientBuilder(76-80)IRestClientBuilder(82-86)IRestClientBuilder(88-92)IRestClientBuilder(94-98)IRestClientBuilder(100-104)IRestClientBuilder(106-110)IRestClientBuilder(112-116)Activout.RestClient.Json/SystemTextJsonSerializer.cs (1)
SystemTextJsonSerializer(11-48)Activout.RestClient.Json/SystemTextJsonDeserializer.cs (1)
SystemTextJsonDeserializer(9-52)
Activout.RestClient.Newtonsoft.Json/RestClientBuilderNewtonsoftJsonExtensions.cs (2)
Activout.RestClient.Newtonsoft.Json/NewtonsoftJsonSerializer.cs (2)
NewtonsoftJsonSerializer(7-31)NewtonsoftJsonSerializer(13-16)Activout.RestClient.Newtonsoft.Json/NewtonsoftJsonDeserializer.cs (2)
NewtonsoftJsonDeserializer(7-39)NewtonsoftJsonDeserializer(13-16)
Activout.RestClient/Implementation/RestClient.cs (1)
Activout.RestClient/Implementation/RequestHandler.cs (4)
Type(151-158)RequestHandler(20-575)RequestHandler(45-113)Send(194-240)
Activout.RestClient/Implementation/RestClientBuilder.cs (10)
Activout.RestClient/Implementation/RestClient.cs (2)
RestClient(10-47)RestClient(18-22)Activout.RestClient/MediaType.cs (2)
MediaType(8-11)MediaType(15-22)Activout.RestClient/Helpers/Implementation/DuckTyping.cs (1)
DuckTyping(5-13)Activout.RestClient/Serialization/Implementation/SerializationManager.cs (4)
ISerializer(43-47)SerializationManager(8-48)SerializationManager(30-35)IDeserializer(37-41)Activout.RestClient/Helpers/Implementation/TaskConverter3Factory.cs (1)
TaskConverter3Factory(5-15)Activout.RestClient/ParamConverter/Implementation/ParamConverterManager.cs (2)
ParamConverterManager(7-31)ParamConverterManager(13-17)Activout.RestClient/Implementation/DummyRequestLogger.cs (1)
DummyRequestLogger(6-19)Activout.RestClient/ContentTypeAttribute.cs (1)
ContentTypeAttribute(10-13)Activout.RestClient/DomainExceptions/DomainExceptionAttribute.cs (1)
DomainExceptionAttribute(10-18)Activout.RestClient/Implementation/HeaderListExtensions.cs (1)
AddOrReplaceHeader(8-18)
Activout.RestClient/Implementation/RestClientContext.cs (1)
Activout.RestClient/Implementation/RestClient.cs (2)
RestClient(10-47)RestClient(18-22)
Activout.RestClient/Serialization/Implementation/StringSerializer.cs (3)
Activout.RestClient/Serialization/Implementation/SerializationManager.cs (1)
ISerializer(43-47)Activout.RestClient/Serialization/ISerializer.cs (2)
HttpContent(9-9)CanSerialize(10-10)Activout.RestClient/Implementation/RequestHandler.cs (2)
HttpContent(427-434)HttpContent(436-449)
🔇 Additional comments (3)
Activout.RestClient.Newtonsoft.Json.Test/RestClientTests.cs (1)
93-93: LGTM! Good use of discard pattern.Using
_for the unused lambda parameter explicitly indicates it's intentionally unused, which improves code clarity and is consistent with the pattern at line 76.Activout.RestClient.Newtonsoft.Json/RestClientBuilderNewtonsoftJsonExtensions.cs (1)
1-3: Remove the suggestion to addusing System.Linq;—LINQ is already available via implicit usings.The project has
<ImplicitUsings>enable</ImplicitUsings>configured for net8.0, which automatically includesSystem.Linqwithout an explicit import. The.Select()and.First()calls work correctly as-is. No changes needed.Likely an incorrect or invalid review comment.
Activout.RestClient.Json/RestClientBuilderJsonExtensions.cs (1)
1-3: LINQ import is not needed; System.Linq is already available through ImplicitUsings.With .NET 8.0 and
<ImplicitUsings>enable</ImplicitUsings>configured in the project file,System.Linqis automatically included globally. The code usesSelect()andFirst()methods without issue, confirming that the namespace is in scope through implicit usings rather than explicit import.Likely an incorrect or invalid review comment.
| var method = _type.GetMethod(binder.Name); | ||
| if (method == null) | ||
| { | ||
| return _type.GetMembers().Select(x => x.Name); | ||
| result = null; | ||
| return false; | ||
| } |
There was a problem hiding this comment.
Fix overload resolution to avoid AmbiguousMatchException and wrong method dispatch
_type.GetMethod(binder.Name) throws for overloaded methods and ignores argument shapes. Resolve by selecting the best candidate using argument count/types (and allowing a trailing CancellationToken).
Apply this diff to use a resolver:
- var method = _type.GetMethod(binder.Name);
- if (method == null)
+ var method = ResolveMethod(binder.Name, args);
+ if (method is null)
{
result = null;
return false;
}Add this helper inside the class:
private MethodInfo? ResolveMethod(string name, object?[]? args)
{
var candidates = _type.GetMethods(BindingFlags.Public | BindingFlags.Instance)
.Where(m => m.Name == name)
.ToArray();
if (candidates.Length == 0) return null;
var argArray = args as object[] ?? Array.Empty<object>();
var argCount = argArray.Length;
foreach (var m in candidates)
{
var ps = m.GetParameters();
var hasCt = ps.Length > 0 && ps[^1].ParameterType == typeof(System.Threading.CancellationToken);
var matchCount = hasCt ? ps.Length - 1 : ps.Length;
if (matchCount != argCount) continue;
var ok = true;
for (var i = 0; i < matchCount; i++)
{
var a = argArray[i];
if (a is null) continue; // null is acceptable for ref/nullable params
if (!ps[i].ParameterType.IsInstanceOfType(a)) { ok = false; break; }
}
if (ok) return m;
}
// Fallback: first by name
return candidates.FirstOrDefault();
}And add the needed using:
using System.Reflection;
+using System.Threading;🤖 Prompt for AI Agents
In Activout.RestClient/Implementation/RestClient.cs around lines 31 to 36, the
current call to _type.GetMethod(binder.Name) fails for overloaded methods and
ignores argument shapes; add a ResolveMethod helper (as described in the review)
inside the class to pick the correct MethodInfo by filtering candidates by name,
parameter count/types and allowing a trailing CancellationToken, replace the
direct GetMethod call with ResolveMethod(binder.Name, args) (or similar) so you
select the best overload, and add the required using for BindingFlags/Reflection
at the top of the file.
| result = requestHandler.Send(args); | ||
| return true; |
There was a problem hiding this comment.
Null-safe handoff of args to RequestHandler.Send(object[])
TryInvokeMember receives object?[]?; RequestHandler.Send expects object[]. Coalesce to an empty array at the call site.
- result = requestHandler.Send(args);
+ result = requestHandler.Send((args as object[]) ?? Array.Empty<object>());
return true;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| result = requestHandler.Send(args); | |
| return true; | |
| result = requestHandler.Send((args as object[]) ?? Array.Empty<object>()); | |
| return true; |
🤖 Prompt for AI Agents
In Activout.RestClient/Implementation/RestClient.cs around lines 44-45,
TryInvokeMember receives a nullable object?[]? named args but
RequestHandler.Send expects a non-null object[]; coalesce args to
Array.Empty<object>() (or new object[0]) at the call site so you pass a non-null
empty array when args is null, and then call requestHandler.Send with that
coalesced array.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (7)
Activout.RestClient/Implementation/DummyRequestLogger.cs (2)
6-6: Consider making the constructor private to enforce the singleton pattern.While the class is already
internal sealed, making the constructor private would more explicitly communicate that direct instantiation is not intended and theInstanceproperty should be used.Apply this diff to add a private constructor:
internal sealed class DummyRequestLogger : IRequestLogger, IDisposable { + private DummyRequestLogger() { } + public static IRequestLogger Instance { get; } = new DummyRequestLogger();
12-15: Consider removing the comment.The comment explains what the code does (nothing) rather than why, which doesn't align with the coding guideline. Since the method body is empty, it's self-evident that it does nothing.
As per coding guidelines.
Apply this diff to remove the comment:
public void Dispose() { - // Do nothing }Activout.RestClient/Serialization/Implementation/StringSerializer.cs (2)
11-11: Avoid singleton-wide side effects from mutable Order.If callers mutate Order on the shared Instance, it affects all clients. Prefer making Order immutable (if the ISerializer contract allows) or ensure consumers don’t adjust it.
Example:
- public int Order { get; set; } = 1000; + public int Order { get; } = 1000;If ISerializer requires a setter, consider not exposing a singleton for configurable instances and use the singleton only with a fixed order.
7-18: Use the singleton consistently in defaults.SerializationManager.DefaultSerializers still constructs a new StringSerializer(). To avoid duplicate instances and ensure consistent ordering, switch it to StringSerializer.Instance.
Change in Activout.RestClient/Serialization/Implementation/SerializationManager.cs:
- new FormUrlEncodedSerializer(), - new StringSerializer(), - new ByteArraySerializer() + new FormUrlEncodedSerializer(), + StringSerializer.Instance, + new ByteArraySerializer()Activout.RestClient/IRestClientBuilder.cs (1)
1-1: Enable nullable on the public surface.Public API should ship with nullable enabled for better consumers’ experience.
-#nullable disable +#nullable enableActivout.RestClient/Implementation/HeaderListExtensions.cs (1)
8-18: LGTM; consider argument guards as a small hardening.Current logic is fine. Optionally add null checks to fail fast.
public static void AddOrReplaceHeader<T>(this List<KeyValuePair<string, T>> headers, string name, T value, bool replace) { - if (replace) + if (headers == null) throw new ArgumentNullException(nameof(headers)); + if (name == null) throw new ArgumentNullException(nameof(name)); + if (replace) { headers.RemoveAll(header => header.Key.Equals(name, StringComparison.OrdinalIgnoreCase)); } headers.Add(new KeyValuePair<string, T>(name, value)); }Activout.RestClient/Implementation/RestClientBuilder.cs (1)
118-147: HttpClient lifetime consideration.Creating a new HttpClient per Build when none is supplied can lead to socket exhaustion in some usage patterns. Prefer requiring an HttpClient or integrating with IHttpClientFactory in hosting scenarios.
If keeping the fallback, document usage and recommend passing a shared HttpClient from the app layer.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
Activout.RestClient/IRestClientBuilder.cs(1 hunks)Activout.RestClient/Implementation/DummyRequestLogger.cs(1 hunks)Activout.RestClient/Implementation/HeaderListExtensions.cs(1 hunks)Activout.RestClient/Implementation/RestClientBuilder.cs(2 hunks)Activout.RestClient/Implementation/RestClientContext.cs(1 hunks)Activout.RestClient/Serialization/Implementation/StringSerializer.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/Implementation/RestClientBuilder.csActivout.RestClient/Implementation/RestClientContext.csActivout.RestClient/Implementation/HeaderListExtensions.csActivout.RestClient/Implementation/DummyRequestLogger.csActivout.RestClient/Serialization/Implementation/StringSerializer.csActivout.RestClient/IRestClientBuilder.cs
🧬 Code graph analysis (5)
Activout.RestClient/Implementation/RestClientBuilder.cs (10)
Activout.RestClient/Implementation/RestClient.cs (2)
RestClient(10-47)RestClient(18-22)Activout.RestClient/IRestClientBuilder.cs (12)
IRestClientBuilder(13-13)IRestClientBuilder(14-14)IRestClientBuilder(15-15)IRestClientBuilder(16-16)IRestClientBuilder(17-17)IRestClientBuilder(18-18)IRestClientBuilder(19-19)IRestClientBuilder(20-20)IRestClientBuilder(21-21)IRestClientBuilder(22-22)IRestClientBuilder(23-23)T(24-24)Activout.RestClient/MediaType.cs (2)
MediaType(8-11)MediaType(15-22)Activout.RestClient/Helpers/Implementation/DuckTyping.cs (1)
DuckTyping(5-13)Activout.RestClient/Serialization/Implementation/SerializationManager.cs (4)
ISerializer(43-47)SerializationManager(8-48)SerializationManager(30-35)IDeserializer(37-41)Activout.RestClient/ParamConverter/Implementation/ParamConverterManager.cs (2)
ParamConverterManager(7-31)ParamConverterManager(13-17)Activout.RestClient/Implementation/DummyRequestLogger.cs (1)
DummyRequestLogger(6-16)Activout.RestClient/ContentTypeAttribute.cs (1)
ContentTypeAttribute(10-13)Activout.RestClient/DomainExceptions/DomainExceptionAttribute.cs (1)
DomainExceptionAttribute(10-18)Activout.RestClient/HeaderAttribute.cs (1)
HeaderAttribute(12-17)
Activout.RestClient/Implementation/RestClientContext.cs (5)
Activout.RestClient/Implementation/RestClient.cs (2)
RestClient(10-47)RestClient(18-22)Activout.RestClient/Serialization/ISerializationManager.cs (1)
ISerializer(5-5)Activout.RestClient/Serialization/Implementation/SerializationManager.cs (3)
ISerializer(43-47)SerializationManager(8-48)SerializationManager(30-35)Activout.RestClient/MediaType.cs (2)
MediaType(8-11)MediaType(15-22)Activout.RestClient/ParamConverter/Implementation/ParamConverterManager.cs (2)
ParamConverterManager(7-31)ParamConverterManager(13-17)
Activout.RestClient/Implementation/HeaderListExtensions.cs (1)
Activout.RestClient/Implementation/RestClientBuilder.cs (1)
T(118-147)
Activout.RestClient/Serialization/Implementation/StringSerializer.cs (4)
Activout.RestClient/Serialization/Implementation/SerializationManager.cs (1)
ISerializer(43-47)Activout.RestClient/Serialization/ISerializer.cs (2)
HttpContent(9-9)CanSerialize(10-10)Activout.RestClient/Implementation/RequestHandler.cs (2)
HttpContent(427-434)HttpContent(436-449)Activout.RestClient/MediaType.cs (3)
MediaType(8-11)MediaType(15-22)ToString(24-27)
Activout.RestClient/IRestClientBuilder.cs (3)
Activout.RestClient/Implementation/RestClient.cs (2)
RestClient(10-47)RestClient(18-22)Activout.RestClient/Implementation/RestClientBuilder.cs (15)
IRestClientBuilder(40-44)IRestClientBuilder(46-50)IRestClientBuilder(52-56)IRestClientBuilder(58-62)IRestClientBuilder(64-68)IRestClientBuilder(70-74)IRestClientBuilder(76-80)IRestClientBuilder(82-86)IRestClientBuilder(88-92)IRestClientBuilder(94-98)IRestClientBuilder(100-104)IRestClientBuilder(106-110)IRestClientBuilder(112-116)Uri(174-184)T(118-147)Activout.RestClient/IRestClientFactory.cs (1)
IRestClientBuilder(6-6)
🔇 Additional comments (2)
Activout.RestClient/Implementation/DummyRequestLogger.cs (1)
6-15: LGTM! Clean singleton no-op logger implementation.The implementation is correct and efficient:
- Thread-safe singleton pattern using static property initialization
- Returning
thisfromTimeOperationis an acceptable optimization for a no-op logger, avoiding unnecessary allocations- Expression-bodied members and file-scoped namespace align with coding guidelines
Activout.RestClient/Implementation/RestClientContext.cs (1)
12-29: Immutable context snapshot looks good.Record parameters and IReadOnlyList headers align with immutability goals.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
Activout.RestClient/Implementation/RestClientBuilder.cs (2)
163-163: Nice: defensive copy of headers preserves context immutability.Prevents later builder mutations from affecting built clients. Matches the “record context” intent.
192-196: Guard against empty/whitespace PathAttribute.Template.Prevents accidental empty base templates.
- case PathAttribute pathAttribute: - if (pathAttribute.Template != null) - { - _baseTemplate = pathAttribute.Template; - } + case PathAttribute pathAttribute: + if (!string.IsNullOrWhiteSpace(pathAttribute.Template)) + { + _baseTemplate = pathAttribute.Template; + }
🧹 Nitpick comments (6)
Activout.RestClient/Serialization/Implementation/StringDeserializer.cs (1)
19-22: Consider using expression-bodied syntax for simple method.The method contains a single expression and could be more concise with expression-bodied syntax. As per coding guidelines.
Apply this diff:
- public bool CanDeserialize(MediaType mediaType) - { - return mediaType.Value.StartsWith("text/"); - } + public bool CanDeserialize(MediaType mediaType) => + mediaType.Value.StartsWith("text/");Activout.RestClient/Serialization/Implementation/SerializationManager.cs (1)
10-16: Adopt collection expressions ([]) to match project guidelines.Simplify defaults with C# collection expressions for readability and consistency.
Apply:
- public static readonly IReadOnlyCollection<ISerializer> DefaultSerializers = new List<ISerializer> - { - new FormUrlEncodedSerializer(), - StringSerializer.Instance, - new ByteArraySerializer() - } - .ToImmutableList(); + public static readonly IReadOnlyCollection<ISerializer> DefaultSerializers = + [ new FormUrlEncodedSerializer(), StringSerializer.Instance, new ByteArraySerializer() ] + .ToImmutableList(); - public static readonly IReadOnlyCollection<IDeserializer> DefaultDeserializers = - new List<IDeserializer> - { - new StringDeserializer(), - new ByteArrayDeserializer() - } - .ToImmutableList(); + public static readonly IReadOnlyCollection<IDeserializer> DefaultDeserializers = + [ new StringDeserializer(), new ByteArrayDeserializer() ] + .ToImmutableList();As per coding guidelines.
Also applies to: 18-24
Activout.RestClient/RestClientBuilderExtensions.cs (2)
8-27: Use expression-bodied members for one-liners.Keeps extensions terse and idiomatic.
- public static IRestClientBuilder BaseUri(this IRestClientBuilder self, string apiUri) - { - return self.BaseUri(new Uri(apiUri)); - } + public static IRestClientBuilder BaseUri(this IRestClientBuilder self, string apiUri) => + self.BaseUri(new Uri(apiUri)); - public static IRestClientBuilder ContentType(this IRestClientBuilder self, string contentType) - { - return self.ContentType(new MediaType(contentType)); - } + public static IRestClientBuilder ContentType(this IRestClientBuilder self, string contentType) => + self.ContentType(new MediaType(contentType)); - public static IRestClientBuilder Accept(this IRestClientBuilder self, string accept) - { - return self.Header("Accept", accept); - } + public static IRestClientBuilder Accept(this IRestClientBuilder self, string accept) => + self.Header("Accept", accept); - public static IRestClientBuilder Header(this IRestClientBuilder self, - AuthenticationHeaderValue authenticationHeaderValue) - { - return self.Header("Authorization", authenticationHeaderValue); - } + public static IRestClientBuilder Header(this IRestClientBuilder self, + AuthenticationHeaderValue authenticationHeaderValue) => + self.Header("Authorization", authenticationHeaderValue);As per coding guidelines.
23-27: Name the Authorization helper explicitly.Method name Header(AuthenticationHeaderValue) is vague and easy to confuse with Header(name,value). Consider renaming to Authorization to clarify intent.
- public static IRestClientBuilder Header(this IRestClientBuilder self, + public static IRestClientBuilder Authorization(this IRestClientBuilder self, AuthenticationHeaderValue authenticationHeaderValue) => self.Header("Authorization", authenticationHeaderValue);Activout.RestClient/Implementation/RestClientBuilder.cs (2)
161-165: Use singleton DefaultDomainExceptionMapperFactory.Instance.Avoid unnecessary allocations; align with other singletons (DuckTyping, ParamConverterManager, DummyRequestLogger).
- DomainExceptionMapperFactory: _domainExceptionMapperFactory ?? - new DefaultDomainExceptionMapperFactory(), + DomainExceptionMapperFactory: _domainExceptionMapperFactory ?? + DefaultDomainExceptionMapperFactory.Instance,
155-156: Operational note: consider HttpClient lifetime.Defaulting to new HttpClient per Build can increase socket pressure in apps that build many clients. Encourage consumers to pass a shared HttpClient (e.g., via HttpClientFactory), or consider an optional static default.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
Activout.RestClient.Newtonsoft.Json.Test/NewtonsoftJsonDeserializerTest.cs(4 hunks)Activout.RestClient/IRestClientBuilder.cs(1 hunks)Activout.RestClient/Implementation/DummyRequestLogger.cs(1 hunks)Activout.RestClient/Implementation/RestClientBuilder.cs(2 hunks)Activout.RestClient/RestClientBuilderExtensions.cs(1 hunks)Activout.RestClient/Serialization/IDeserializer.cs(1 hunks)Activout.RestClient/Serialization/ISerializer.cs(1 hunks)Activout.RestClient/Serialization/Implementation/SerializationManager.cs(2 hunks)Activout.RestClient/Serialization/Implementation/StringDeserializer.cs(1 hunks)Activout.RestClient/Serialization/Implementation/StringSerializer.cs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- Activout.RestClient/Serialization/Implementation/StringSerializer.cs
- Activout.RestClient/Implementation/DummyRequestLogger.cs
🧰 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/Serialization/ISerializer.csActivout.RestClient.Newtonsoft.Json.Test/NewtonsoftJsonDeserializerTest.csActivout.RestClient/Implementation/RestClientBuilder.csActivout.RestClient/Serialization/IDeserializer.csActivout.RestClient/Serialization/Implementation/SerializationManager.csActivout.RestClient/Serialization/Implementation/StringDeserializer.csActivout.RestClient/IRestClientBuilder.csActivout.RestClient/RestClientBuilderExtensions.cs
🧬 Code graph analysis (8)
Activout.RestClient/Serialization/ISerializer.cs (5)
Activout.RestClient/Serialization/Implementation/SerializationManager.cs (1)
ISerializer(43-47)Activout.RestClient.Newtonsoft.Json/NewtonsoftJsonSerializer.cs (2)
CanSerialize(27-30)Activout(5-32)Activout.RestClient.Json/SystemTextJsonSerializer.cs (2)
CanSerialize(44-47)SystemTextJsonSerializer(11-48)Activout.RestClient.Xml/XmlSerializer.cs (2)
CanSerialize(29-32)XmlSerializer(7-28)Activout.RestClient/Serialization/Implementation/ByteArraySerializer.cs (2)
CanSerialize(25-28)Activout(6-31)
Activout.RestClient.Newtonsoft.Json.Test/NewtonsoftJsonDeserializerTest.cs (2)
Activout.RestClient.Newtonsoft.Json.Test/RestClientTests.cs (1)
IRestClientBuilder(33-42)Activout.RestClient.Newtonsoft.Json.Test/SerializationOrderTest.cs (1)
IRestClientBuilder(77-83)
Activout.RestClient/Implementation/RestClientBuilder.cs (13)
Activout.RestClient/Implementation/RestClient.cs (2)
RestClient(10-47)RestClient(18-22)Activout.RestClient/IRestClientBuilder.cs (14)
IRestClientBuilder(13-13)IRestClientBuilder(14-14)IRestClientBuilder(15-15)IRestClientBuilder(16-16)IRestClientBuilder(17-17)IRestClientBuilder(18-18)IRestClientBuilder(19-19)IRestClientBuilder(20-20)IRestClientBuilder(21-21)IRestClientBuilder(22-22)IRestClientBuilder(23-23)IRestClientBuilder(24-24)IRestClientBuilder(25-25)T(26-26)Activout.RestClient/MediaType.cs (2)
MediaType(8-11)MediaType(15-22)Activout.RestClient/Helpers/Implementation/DuckTyping.cs (1)
DuckTyping(5-13)Activout.RestClient/DomainExceptions/DefaultDomainExceptionMapperFactory.cs (2)
List(30-43)DefaultDomainExceptionMapperFactory(9-90)Activout.RestClient/Serialization/Implementation/SerializationManager.cs (4)
ISerializer(43-47)IDeserializer(37-41)SerializationManager(8-48)SerializationManager(30-35)Activout.RestClient/Implementation/HeaderListExtensions.cs (1)
AddOrReplaceHeader(8-18)Activout.RestClient/Helpers/Implementation/TaskConverter3Factory.cs (1)
TaskConverter3Factory(5-15)Activout.RestClient/ParamConverter/Implementation/ParamConverterManager.cs (2)
ParamConverterManager(7-31)ParamConverterManager(13-17)Activout.RestClient/Implementation/DummyRequestLogger.cs (2)
DummyRequestLogger(6-19)DummyRequestLogger(10-12)Activout.RestClient/ContentTypeAttribute.cs (1)
ContentTypeAttribute(10-13)Activout.RestClient/DomainExceptions/DomainExceptionAttribute.cs (1)
DomainExceptionAttribute(10-18)Activout.RestClient/HeaderAttribute.cs (1)
HeaderAttribute(12-17)
Activout.RestClient/Serialization/IDeserializer.cs (5)
Activout.RestClient/Serialization/Implementation/SerializationManager.cs (1)
IDeserializer(37-41)Activout.RestClient/Serialization/ISerializationManager.cs (1)
IDeserializer(6-6)Activout.RestClient/Serialization/Implementation/StringDeserializer.cs (1)
CanDeserialize(19-22)Activout.RestClient/Serialization/Implementation/ByteArrayDeserializer.cs (1)
CanDeserialize(36-39)Activout.RestClient/MediaType.cs (2)
MediaType(8-11)MediaType(15-22)
Activout.RestClient/Serialization/Implementation/SerializationManager.cs (1)
Activout.RestClient/Serialization/Implementation/StringSerializer.cs (1)
StringSerializer(7-18)
Activout.RestClient/Serialization/Implementation/StringDeserializer.cs (4)
Activout.RestClient/Serialization/Implementation/SerializationManager.cs (1)
IDeserializer(37-41)Activout.RestClient/Serialization/IDeserializer.cs (2)
Task(10-10)CanDeserialize(11-11)Activout.RestClient/Implementation/RequestHandler.cs (7)
Task(452-509)Task(511-546)Task(548-559)Task(561-574)HttpContent(427-434)HttpContent(436-449)Type(151-158)Activout.RestClient/MediaType.cs (2)
MediaType(8-11)MediaType(15-22)
Activout.RestClient/IRestClientBuilder.cs (2)
Activout.RestClient/Implementation/RestClientBuilder.cs (15)
IRestClientBuilder(45-49)IRestClientBuilder(51-55)IRestClientBuilder(57-61)IRestClientBuilder(63-67)IRestClientBuilder(69-73)IRestClientBuilder(75-79)IRestClientBuilder(81-85)IRestClientBuilder(87-96)IRestClientBuilder(98-107)IRestClientBuilder(109-119)IRestClientBuilder(121-125)IRestClientBuilder(127-131)IRestClientBuilder(133-137)Uri(200-210)T(139-170)Activout.RestClient/RestClientBuilderExtensions.cs (2)
IRestClientBuilder(8-11)IRestClientBuilder(13-16)
Activout.RestClient/RestClientBuilderExtensions.cs (3)
Activout.RestClient/IRestClientBuilder.cs (13)
IRestClientBuilder(13-13)IRestClientBuilder(14-14)IRestClientBuilder(15-15)IRestClientBuilder(16-16)IRestClientBuilder(17-17)IRestClientBuilder(18-18)IRestClientBuilder(19-19)IRestClientBuilder(20-20)IRestClientBuilder(21-21)IRestClientBuilder(22-22)IRestClientBuilder(23-23)IRestClientBuilder(24-24)IRestClientBuilder(25-25)Activout.RestClient/Implementation/RestClientBuilder.cs (4)
IRestClientBuilder(45-49)IRestClientBuilder(51-55)IRestClientBuilder(57-61)Uri(200-210)Activout.RestClient/MediaType.cs (2)
MediaType(8-11)MediaType(15-22)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (csharp)
🔇 Additional comments (13)
Activout.RestClient/Serialization/Implementation/StringDeserializer.cs (3)
5-6: LGTM: File-scoped namespace applied correctly.The file-scoped namespace declaration aligns with the coding guidelines.
7-7: LGTM: Appropriate access modifier for implementation class.Making the class internal is correct since it's an implementation detail, while the public interface
IDeserializerremains accessible.
9-9: LGTM: Init-only Order property provides immutability.The
initaccessor correctly satisfies the interface's get-only requirement while allowing initialization-time configuration and preventing post-construction mutation.Activout.RestClient/Serialization/IDeserializer.cs (2)
5-6: LGTM: File-scoped namespace applied correctly.The file-scoped namespace declaration aligns with the coding guidelines.
9-9: LGTM: Read-only Order property enforces immutability.Changing the Order property to get-only aligns with the PR objective of making components more immutable. Implementations remain compatible using
{ get; set; }or{ get; init; }accessors.Activout.RestClient/Serialization/ISerializer.cs (2)
4-5: LGTM: File-scoped namespace applied correctly.The file-scoped namespace declaration aligns with the coding guidelines and is consistent with the other interface changes in this PR.
8-8: LGTM: Read-only Order property enforces immutability.Changing the Order property to get-only mirrors the IDeserializer change and aligns with the PR objective of making components more immutable. All observed implementations remain compatible with
{ get; set; }accessors.Activout.RestClient.Newtonsoft.Json.Test/NewtonsoftJsonDeserializerTest.cs (3)
44-46: LGTM: test payloads clearly exercise Pascal vs camel case.Anonymous objects match expected JSON property casing; assertions read clean.
Also applies to: 65-67
93-101: Good rename and use of shared CamelCase settings.Using NewtonsoftJsonDefaults.CamelCaseSerializerSettings keeps tests aligned with library defaults.
110-116: Constructor overload improves test ergonomics.Optional JsonSerializerSettings passthrough to WithNewtonsoftJson is tidy and explicit.
Activout.RestClient/Serialization/Implementation/SerializationManager.cs (1)
10-16: Switch to StringSerializer.Instance is correct.Singleton avoids unnecessary allocations and matches new StringSerializer design.
Activout.RestClient/IRestClientBuilder.cs (2)
15-16: Header default replace=true aligns with HeaderAttribute.Consistent semantics avoid duplicate headers when mixing attributes and builder calls.
25-26: Interface parity: ParamConverter manager exposed.Keeps IRestClientBuilder aligned with implementation and extensions.
1 participant
Summary by CodeRabbit
New Features
Improvements
Improvements / API
Chores