v2: updated client #72
Conversation
# Conflicts: # src/tests/Tochka.JsonRpc.Client.Tests.Integration/IntegrationTests.cs # src/tests/Tochka.JsonRpc.Client.Tests.Integration/Tochka.JsonRpc.Client.Tests.Integration.csproj
| /// <remarks> | ||
| /// Default is "Tochka.JsonRpc.Client" | ||
| /// </remarks> | ||
| public virtual string UserAgent => typeof(JsonRpcClientBase).Namespace!; |
There was a problem hiding this comment.
nit, may be cache in static this useless reflection?
| /// <param name="cancellationToken"></param> | ||
| /// <returns>Raw HTTP response</returns> | ||
| /// <exception cref="System.ArgumentException">When requestUrl starts with '/'</exception> | ||
| Task<HttpResponseMessage> Send(string requestUrl, ICall call, CancellationToken cancellationToken); |
There was a problem hiding this comment.
for what?
if we don't need response - it's a SendNotification, i suppose...
There was a problem hiding this comment.
Updated doc to clarify what is this method for.
Also added simmilar methods for batch, that were missing
| default: | ||
| var message = $"Expected single response, got [{responseWrapper}]"; | ||
| Log.LogTrace("Request id [{requestId}] failed: {errorMessage}", request.Id, message); | ||
| throw new JsonRpcException(message, context); |
There was a problem hiding this comment.
nit for inline optimization and standardization prefere to use throw helper method in libs
https://learn.microsoft.com/en-us/dotnet/communitytoolkit/diagnostics/throwhelper
or custom.
it applies to the whole project
There was a problem hiding this comment.
It spoils compiler checks for nullable types and asks for additional break/return statements. Not worth it.
| { | ||
| var context = CreateContext(); | ||
| context.WithRequestUrl(requestUrl); | ||
| var data = calls.Select(x => x.WithSerializedParams(DataJsonSerializerOptions)).ToList(); |
There was a problem hiding this comment.
i suppose list is overkill type for this collection. You don't need mutate this, right?
| /// Set HttpClient properties from base options | ||
| /// </summary> | ||
| [ExcludeFromCodeCoverage] | ||
| protected internal virtual void InitializeClient(HttpClient client, JsonRpcClientOptionsBase options) |
| namespace Tochka.JsonRpc.Client.Models; | ||
|
|
||
| [PublicAPI] | ||
| public interface ISingleJsonRpcResult |
There was a problem hiding this comment.
will be added later with all other documentation
| throw new InvalidOperationException("Can not add batch response when no response is expected"); | ||
| } | ||
|
|
||
| BatchResponse = batchResponse; |
There was a problem hiding this comment.
to bottom of method
assignation gets lost here
| namespace Tochka.JsonRpc.Client.Models; | ||
|
|
||
| [PublicAPI] | ||
| public class JsonRpcCallContext : IJsonRpcCallContext |
| public T? GetResponseOrThrow<T>() | ||
| { | ||
| switch (response) | ||
| { |
There was a problem hiding this comment.
Wasn't possible because of context.WithError call.
Added chaining to context and changed it
✅
|
|
||
| public class JsonRpcIdGenerator : IJsonRpcIdGenerator | ||
| { | ||
| private readonly ILogger<JsonRpcIdGenerator> log; |
There was a problem hiding this comment.
nit, this is too much. instal log class for what? One unneeded log?
| @@ -0,0 +1,6 @@ | |||
| namespace Tochka.JsonRpc.Common.Models.Id; | |||
|
|
|||
| public record NullRpcId : IRpcId | |||
| @@ -0,0 +1,6 @@ | |||
| namespace Tochka.JsonRpc.Common.Models.Id; | |||
|
|
|||
| public record NumberRpcId(long Value) : IRpcId | |||
| @@ -0,0 +1,6 @@ | |||
| namespace Tochka.JsonRpc.Common.Models.Id; | |||
|
|
|||
| public record StringRpcId(string Value) : IRpcId | |||
|
|
||
| <ItemGroup> | ||
| <PackageReference Include="JetBrains.Annotations" Version="2022.3.1" /> | ||
| <PackageReference Include="Yoh.Text.Json.NamingPolicies" Version="1.0.0" /> |
There was a problem hiding this comment.
maybe depending on an external package with a small number of stars is a bad idea?
There was a problem hiding this comment.
It is the best variant that exists. Unfortunatly System.Text.Json doesn't support snake_case for two versions of .net already and it will be added only in .net8 (I hope). This library is full copy of PR that Microsoft accepted to System.Text.Json (but not released yet)
|
|
||
| <IsPackable>false</IsPackable> |
There was a problem hiding this comment.
Don't understand what's wrong here
No description provided.