Address a few small nits noticed while browsing#1158
Conversation
| } | ||
|
|
||
| return Utilities.ToCamelCase(name); | ||
| return JsonNamingPolicy.CamelCase.ConvertName(name); |
There was a problem hiding this comment.
This one isn't 100% identical. The System.Text.Json method is almost exactly the same as the ToCamelCase utility being replaced, except that the STJ method looks for ' ' as a separator whereas the ToCamelCase routine looks for anything that's char.IsSeparator. If that difference is important, this part can be reverted.
There was a problem hiding this comment.
The separator is of no concern since we're translating .NET identifiers, which have no whitespace.
What is possibly an issue however is that you're adding an STJ dependency in code that runs in non-STJ scenarios, requiring STJ to be loaded where it may not have been before. Visual Studio counts assembly loads, so we deliberately avoid dependencies on particular serialization libraries unless we're in code that already is specific to that library.
There was a problem hiding this comment.
Interesting, so it's in the transitive graph here but you're able to avoid loading it by avoiding it in some code paths? How are regressions caught other than by your watchful code review eye? After the fact by the perf tooling?
There was a problem hiding this comment.
Yes, if nothing else than our Perf DDRITs will likely catch it. (or, miss it, if STJ happened to have already loaded for other reasons, but we're actively trying to consolidate serialization technologies to reduce wasted assembly loads).
We also have at least one test that uses AppDomains to verify that we don't load unrelated serialization libraries, but IIRC it's off in our PR builds because it's not entirely stable.
StreamJsonRpc has 3 serialization library dependencies (Newtonsoft.Json, MessagePack-CSharp, and STJ). And it may pick up a 4th by the end of the year. It's unfortunate that in .NET we have to choose between having 3 assemblies (StreamJsonRpc, a serialization library, plus a binding library) or taking a hard dependency in StreamJsonRpc to reduce this to 2 assemblies. I know AOT resolves this, for those apps that can do AOT and for libraries that are AOT safe (StreamJsonRpc is not -- yet). So we try to get the best of both worlds by taking the hard dependency and then avoiding references to it outside of code that really requires it.
The NetArchTest.Rules nuget package includes the ability to functionally prove that certain types or namespaces take no dependencies on other areas (e.g. assemblies). I only recently discovered it and it may help us enforce dependency compartmentalizing for StreamJsonRpc via our functional unit tests.
AArnott
left a comment
AArnott
left a comment
There was a problem hiding this comment.
Thank you very much for your insights and fixes!
|
|
||
| // Now go back and fill in the header with the actual content length. | ||
| Utilities.Write(this.Writer.GetSpan(sizeof(int)), checked((int)contentSequence.Length)); | ||
| BinaryPrimitives.WriteInt32BigEndian(this.Writer.GetSpan(sizeof(int)), checked((int)contentSequence.Length)); |
There was a problem hiding this comment.
Is this another one that's going to cause you to load a library you didn't want to?
Hmm, probably not, since you're already using span.
There was a problem hiding this comment.
Thanks for checking. Ya, we're using System.Memory heavily throughout the library.
| if (rpcInterfaceCode.HasValue) | ||
| { | ||
| methodName = rpcInterfaceCode + "." + method.Name; | ||
| rpcMethodName = rpcInterfaceCode + "." + rpcMethodName; | ||
| methodName = $"{rpcInterfaceCode.GetValueOrDefault()}.{method.Name}"; | ||
| rpcMethodName = $"{rpcInterfaceCode.GetValueOrDefault()}.{rpcMethodName}"; | ||
| } |
There was a problem hiding this comment.
if (rpcInterfaceCode is { } localRpcInterfaceCode)
{
methodName = $"{localRpcInterfaceCode}.{method.Name}";
rpcMethodName = $"{localRpcInterfaceCode}.{rpcMethodName}";
}/cc @stephentoub 😄
There was a problem hiding this comment.
Or why not just:
if (rpcInterfaceCode.HasValue)
{
methodName = $"{rpcInterfaceCode}.{method.Name}";
rpcMethodName = $"{rpcInterfaceCode}.{rpcMethodName}";
}There was a problem hiding this comment.
For the latter, just because nullable value types are a bit less efficient in interpolated strings. Probably not a big deal, though.
For the former, seems like a stylistic preference.
There was a problem hiding this comment.
It's exactly the same code, but more CSharpy. 😄
It just shows you were used to doing the right thing before C# had pattern matching. 😄
3 participants
No description provided.