Codecov Report

Attention: Patch coverage is 85.41667% with 7 lines in your changes missing coverage. Please review.

Project coverage is 91.73%. Comparing base (75070da) to head (f6232a7).
Report is 3 commits behind head on dev.

@@            Coverage Diff             @@
##              dev   #10411      +/-   ##
==========================================
+ Coverage   91.60%   91.73%   +0.13%     
==========================================
  Files         427      427              
  Lines       13368    13375       +7     
  Branches     2580     2581       +1     
==========================================
+ Hits        12246    12270      +24     
+ Misses        544      531      -13     
+ Partials      578      574       -4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Add these to BindingConverterTests.cs should get you closer to 100% coverage. Mostly just missing Error testing. Which Looks like Converter.cs instead of setting SetErrorMessage and GetErrorMessage directly the error messages should be calling UpdateSetErrorMessage() and UpdateGetErrorMessage() respectively.

        [Test]
        public void DefaultConverter_TimeSpan_EdgeCases()
        {
            var converter = new DefaultConverter<TimeSpan>();

            // Test invalid format
            converter.Get("25:00").Should().Be(TimeSpan.Zero); // Invalid hours
            converter.GetError.Should().BeTrue();
            converter.GetErrorMessage.Should().Be("Not a valid time span");

            // Test overflow
            converter.Get("999999:00:00").Should().Be(TimeSpan.Zero);
            converter.GetError.Should().BeTrue();
            converter.GetErrorMessage.Should().Be("Not a valid time span");

            // Test invalid characters
            converter.Get("12:ab:00").Should().Be(TimeSpan.Zero);
            converter.GetError.Should().BeTrue();
            converter.GetErrorMessage.Should().Be("Not a valid time span");

            // Test empty/null
            converter.Get("").Should().Be(TimeSpan.Zero);
            converter.Get(null).Should().Be(TimeSpan.Zero);
        }

        [Test]
        public void DefaultConverter_TimeSpanFormat_CustomFormats()
        {
            var converter = new DefaultConverter<TimeSpan>();

            // Test default format
            var time = new TimeSpan(1, 2, 3);
            converter.Set(time).Should().Be("01:02:03");
            converter.Get("01:02:03").Should().Be(time);

            // Test custom DefaultTimeSpanFormat
            converter.DefaultTimeSpanFormat = @"hh\:mm";
            converter.Set(time).Should().Be("01:02");
            converter.Get("01:02").Should().Be(new TimeSpan(1, 2, 0));

            // Test Format property override
            converter.Format = @"hh\:mm\:ss\.fff";
            var preciseTime = new TimeSpan(0, 1, 2, 3, 456);
            converter.Set(preciseTime).Should().Be("01:02:03.456");
            converter.Get("01:02:03.456").Should().Be(preciseTime);
        }

        [Test]
        public void BaseConverter_Culture_AffectsConversion()
        {
            var cultureInfo = new CultureInfo("fr-FR");
            var converter = new Converter<double, string>
            {
                Culture = cultureInfo,
                SetFunc = (d) => d.ToString(cultureInfo),
                GetFunc = (s) => double.Parse(s, cultureInfo)
            };

            converter.Set(1.5).Should().Be("1,5"); // French uses comma
            converter.Get("1,5").Should().Be(1.5);

            cultureInfo = new CultureInfo("en-US");
            converter = new Converter<double, string>
            {
                Culture = cultureInfo,
                SetFunc = (d) => d.ToString(cultureInfo),
                GetFunc = (s) => double.Parse(s, cultureInfo)
            };

            converter.Set(1.5).Should().Be("1.5"); // US uses period
            converter.Get("1.5").Should().Be(1.5);
        }

        [Test]
        public void BaseConverter_NullFuncs_ReturnsDefaults()
        {
            var converter = new Converter<int, string>();
            converter.Set(42).Should().BeNull(); // SetFunc is null
            converter.Get("42").Should().Be(0);  // GetFunc is null
            converter.SetError.Should().BeFalse();
            converter.GetError.Should().BeFalse();
        }

        [Test]
        public void BaseConverter_OnError_CallbackInvoked()
        {
            // Variable to capture the last error message from OnError callback
            var lastError = string.Empty;

            // Create a converter with intentionally failing SetFunc and GetFunc
            var converter = new Converter<int, string>
            {
                OnError = (msg) => lastError = msg, // Assign OnError callback
                SetFunc = (i) => throw new Exception("Set failed"),
                GetFunc = (s) => throw new Exception("Get failed")
            };

            // Test the Set method
            converter.Set(42);
            lastError.Should().Contain("Set failed");
            converter.SetErrorMessage.Should().Contain("Set failed");
            converter.SetError.Should().BeTrue();

            // Test the Get method
            converter.Get("42");
            lastError.Should().Contain("Get failed");
            converter.GetError.Should().BeTrue();
            converter.GetErrorMessage.Should().Contain("Get failed");
        }

Thanks for the PR.

I'm not sure I'm a fan of this approach. The design seems off, with attaching the localizer to the Converter from the side.

First, converters are something I plan to rewrite sooner or later. I want to make them more like in WPF, where the culture is part of the From/To methods, rather than being just a side property, as we have with the localizer here. Also, I'm not a fan of the universal converter that attempts to handle all conversions. As well I don't like that it has this OnError things, it should just throw and someone must just catch. I think the idea was primarily to support inline conversions in Razor syntax with getter and setters, but at this point, I'm not sure it's worth it, after all you could just write a decorator on top of normal conversion for this rather than making it a core logic.

Second, I would approach the translation differently. I wouldn't translate it at the Converter level, so it wouldn't depend on the localizer. Instead, I would spit out the translation keys

-UpdateGetError(Localizer?[LanguageResource.DefaultConverter_InvalidBoolean] ?? "Not a valid boolean");
+UpdateGetError(LanguageResource.DefaultConverter_InvalidBoolean);

And have the components that listen for the OnError event of the converters

+Localizer[error] / Localizer[_converter.GetErrorMessage]

This would feel more natural.

@ScarletKuro Thank you for the review!

Your solution indeed looks cleaner. However, I have a question regarding handling strings with interpolation. For example:

I initially considered changing

to something like a Tuple. This approach doesn’t feel entirely appropriate either. Do you have any recommendations for handling this scenario?

Alternatively, I could change the existing behavior in

and implement a custom solution, but that also seems strange.

I guess we could do a breaking change and make it

public Action<string, object[]>? OnError { get; set; }
public (string?, object[]) GetErrorMessage { get; set; }
protected void UpdateGetError(string msg, object[] arguments)

?

Feedback applied. I hope it's better now.

I also made two additional changes since the last update:

• I changed the prefix for the keys from DefaultConverter to Converter to make it more general.
• I took inspiration from versile2's suggestion and reorganized the unit tests in BindingConverterTests. Instead of having one method for the happy cases and one for negative cases, I split the tests by type. (I hope this change will help a bit for the future reworks of the converters.)

@henon, I’m not sure who else needs to review this.
I don't think many people are using the converter's OnError functionality, so I guess we could add it to v8?

@ScarletKuro As you can see from the issue number, this issue has existed for a long time. I know this is needed, so I'd say let's go for it.

I know that @HClausing is using converters. @HClausing what do you think about merging this in v8?

@pwasilewski can you write a sentence for our migration guide that advises users how they should adapt their code to this breaking change?

@henon
Sure! Does something like this work?

In Converter.cs, several property signatures have been updated to enhance localization support. These properties no longer return English messages directly. Instead, they now return keys intended for localization, accompanied by additional parameters:

• OnError: Updated from Action<string>? to Action<string, object[]>?. This allows passing a key and optional parameters, enabling more flexible and accurate localization.
• SetErrorMessage and GetErrorMessage: Both changed from string? to (string, object[])?. These properties now provide a key for localization along with relevant parameters, shifting message formatting to the localization system.

Quality Gate passed

Issues
6 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

Added to v8.0.0 Migration Guide #9953

@ScarletKuro As you can see from the issue number, this issue has existed for a long time. I know this is needed, so I'd say let's go for it.

I know that @HClausing is using converters. @HClausing what do you think about merging this in v8?

@pwasilewski can you write a sentence for our migration guide that advises users how they should adapt their code to this breaking change?

I really like this approach.

Yes, I've written some custom Converters to dynamically apply masks for brazilian social ID's (companies and natural peopla have different masks, but should be placed on the same field on model/binding).

Would be nice to have it on v8.

Thanks @pwasilewski !