Introduce the new endpoint to the Calinga.NET package#48
Conversation
…e keys are not known to the server
- Add README - Add Author - Add Repository
There was a problem hiding this comment.
The single-string ArgumentNullException constructor expects paramName, not the error message. As-is, the thrown exception reads Value cannot be null. (Parameter 'Parameter cannot be null, empty, or whitespace.') — the actual parameter name is missing and the supposed "message" ends up labelled as the parameter name
|
|
||
| namespace Calinga.NET.Infrastructure.Exceptions | ||
| { | ||
| [Serializable] |
There was a problem hiding this comment.
Quick question about the serialization contract here
The class is marked [Serializable], which I understand signals "this exception supports binary serialization across boundaries". For the other exceptions in this folder (TranslationsNotAvailableException, LanguagesNotAvailableException) that seems fine because they only carry Message and InnerException from the base class. But KeysNotFoundException adds its own
state via MissingKeys.
From what I've read, exceptions with custom state typically need two extra pieces for [Serializable] to fully work:
- A protected deserialization ctor: KeysNotFoundException(SerializationInfo info, StreamingContext context)
- An override of GetObjectData(SerializationInfo, StreamingContext) that writes MissingKeys into the info bag
Both are missing here. My questions:
- Is there a known consumer that serializes this exception across a boundary (legacy remoting, BinaryFormatter, logging sinks, AppDomain hops)? If yes, would MissingKeys silently be lost on the other side as-is?
- Given that BinaryFormatter is obsolete in .NET 5+ and disabled by default in newer TFMs — could we just drop [Serializable] instead of adding the extra ctor + GetObjectData? That would keep the file simple and match what's actually needed in practice.
- Small follow-up: the parameterless and message-only ctors default MissingKeys to Array.Empty(). Is that intentional to avoid null, or would a null (signalling "no list provided") be more useful so callers can distinguish "no keys missing" from "this ctor doesn't carry that info"?
| /// </exception> | ||
| public async Task<IReadOnlyDictionary<string, string>> GetTranslationsAsync(string language) | ||
| { | ||
| return await GetTranslationsAsync(language, false); |
There was a problem hiding this comment.
Small consistency question: every other await in this file uses .ConfigureAwait(false) (I count 12 occurrences), but this one doesn't. I see this overload already existed on master with the same omission, so it's not introduced by this PR — but since we're touching the file anyway, should we add .ConfigureAwait(false) here for consistency?
return await GetTranslationsAsync(language, false).ConfigureAwait(false);
Or is there a reason this one specifically opts out (e.g. it's a thin pass-through and the inner call already has it covered)?
Introduce the key filtering feature to Calinga.NET which we plan to release with the new release.