Skip to content

Converters from AddExtraSettings cannot override built-in converters #1834

Description

@SimonCropp

Converters added via AddExtraSettings currently land behind Verify's own converters, so they can never override a built-in.

SerializationSettings.BuildSettings adds ~25 built-in converters to settings.Converters, and only then runs the extraSettings callbacks:

converters.Add(dateConverter);
converters.Add(timeConverter);
// ...
foreach (var extraSetting in extraSettings)
{
    extraSetting(settings);
}

AddExtraSettings also applies the callback to the already-built jsonSettings, which likewise appends.

Argon resolves converters first-match-wins: JsonSerializer.GetMatchingConverter walks Converters by index and returns the first whose CanConvert returns true. So a user doing the natural thing:

VerifierSettings.AddExtraSettings(settings =>
    settings.Converters.Add(new MyDateOnlyConverter()));

gets a converter that is never invoked, with no error and no warning. Verify's DateConverter keeps winning. This came up in VerifyTests/Argon#394, where the user spent time debugging a DateOnly converter that could not have fired.

The workaround is settings.Converters.Insert(0, ...), but that requires knowing both that Verify preseeds the collection and that Argon is first-match-wins. Neither is discoverable.

Suggestion: have Verify insert extra-settings converters ahead of its built-ins, so user converters take precedence by default. Options:

  1. Run the extraSettings callbacks against the settings before adding the built-in converters, then append the built-ins. Simple, but changes ordering for any other converter-collection mutation the callback performs.
  2. Snapshot converters.Count before running the callbacks and move newly added converters to the front afterwards. More targeted.
  3. Leave the behaviour as is and document it, plus expose something explicit like VerifierSettings.AddConverter that inserts at index 0.

Option 1 or 2 seem preferable to me: a user-registered converter losing to a framework built-in is surprising in either direction, but silently losing is the worse of the two.

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions