Skip to content

Introduce the new endpoint to the Calinga.NET package#48

Open
funkyDev1213 wants to merge 20 commits into
masterfrom
feature/keyFiltering
Open

Introduce the new endpoint to the Calinga.NET package#48
funkyDev1213 wants to merge 20 commits into
masterfrom
feature/keyFiltering

Conversation

@funkyDev1213
Copy link
Copy Markdown

Introduce the key filtering feature to Calinga.NET which we plan to release with the new release.

Comment thread Calinga.NET/Guard.cs Outdated
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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]
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. A protected deserialization ctor: KeysNotFoundException(SerializationInfo info, StreamingContext context)
  2. An override of GetObjectData(SerializationInfo, StreamingContext) that writes MissingKeys into the info bag

Both are missing here. My questions:

  1. 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?
  2. 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.
  3. 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"?

Comment thread Calinga.NET/CalingaService.cs Outdated
/// </exception>
public async Task<IReadOnlyDictionary<string, string>> GetTranslationsAsync(string language)
{
return await GetTranslationsAsync(language, false);
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants