perf: Cleanup allocations + missing ConfigureAwait's

[austindrenski]

Minor stuff, but noticed these while reviewing/evaluating the source for an upcoming project and figured they're most likely oversights, so opening a quick PR to help out.

[austindrenski]

Had inadvertently dropped a break in the original version, but have since fixed that and cleaned up the sign-off and commit title. Should be ready for review and CI checks now.

[toddbaert]

Above, you use string.Equals(url.Scheme, "unix", StringComparison.OrdinalIgnoreCase). Is there a reason to do it differently here?

[austindrenski]

Not necessarily. I'm starting to see the is {const string} pattern more often in the wild for exact/ordinal matching, so since StartsWith(string) implies StartsWith(string, StringComparison.Ordinal), I converted it to a constant ordinal pattern out of habit.

But I can certainly convert it to string.Equals(url.Scheme, "unix", StringComparison.Ordinal) for consistency if you prefer.

[toddbaert]

I have a weak preference for the consistency in this case.

[austindrenski]

Fair enough! Updated:

diff --git a/src/OpenFeature.Contrib.Providers.Flagd/FlagdProvider.cs b/src/OpenFeature.Contrib.Providers.Flagd/FlagdProvider.cs
index 6496ebc..cdb5f26 100644
--- a/src/OpenFeature.Contrib.Providers.Flagd/FlagdProvider.cs
+++ b/src/OpenFeature.Contrib.Providers.Flagd/FlagdProvider.cs
@@ -516,7 +516,7 @@ namespace OpenFeature.Contrib.Providers.Flagd

         private Service.ServiceClient BuildClientForPlatform(Uri url)
         {
-            var useUnixSocket = url.Scheme is "unix";
+            var useUnixSocket = string.Equals(url.Scheme, "unix", StringComparison.Ordinal);

             if (!useUnixSocket)
             {

[toddbaert]

Approving with a question. Thanks again!

Additionally, do you think it's worth adding something like this as we have in the SDK?

[austindrenski]

Approving with a question. Thanks again!

Additionally, do you think it's worth adding something like this as we have in the SDK?

I'm definitely in the enable all analyzers camp, so very much in favor of setting dotnet_diagnostic.CA2007.severity = error in this repo, but didn't want to overstep on this PR.

[austindrenski]

Confused on the test re-failures, given they passed locally last week. I'll re-re-run them locally and see what's going on...

[toddbaert]

Confused on the test re-failures, given they passed locally last week. I'll re-re-run them locally and see what's going on...

NSubstitute.Exceptions.ReceivedCallsException : Expected to receive exactly 1 call matching:
	EventStream(any Empty, <null>, <null>, System.Threading.CancellationToken)
Actually received 1211 matching calls:
	EventStream({ }, <null>, <null>, System.Threading.CancellationToken)

@austindrenski I guess this is a side-effect of adding ConfigureAwait?

[austindrenski]

Confused on the test re-failures, given they passed locally last week. I'll re-re-run them locally and see what's going on...

NSubstitute.Exceptions.ReceivedCallsException : Expected to receive exactly 1 call matching:
	EventStream(any Empty, <null>, <null>, System.Threading.CancellationToken)
Actually received 1211 matching calls:
	EventStream({ }, <null>, <null>, System.Threading.CancellationToken)

@austindrenski I guess this is a side-effect of adding ConfigureAwait?

Locally it seemed to be from that erroneously dropped break from the switch => if/else sequence, so re-re-testing/-reviewing that change now...

[austindrenski]

Confused on the test re-failures, given they passed locally last week. I'll re-re-run them locally and see what's going on...

NSubstitute.Exceptions.ReceivedCallsException : Expected to receive exactly 1 call matching:
	EventStream(any Empty, <null>, <null>, System.Threading.CancellationToken)
Actually received 1211 matching calls:
	EventStream({ }, <null>, <null>, System.Threading.CancellationToken)

@austindrenski I guess this is a side-effect of adding ConfigureAwait?

Locally it seemed to be from that erroneously dropped break from the switch => if/else sequence, so re-re-testing/-reviewing that change now...

doh! mis-converted the original switch by retaining the break statements in the first place.

updated the pr, but interesting bit is here:

diff --git a/src/OpenFeature.Contrib.Providers.Flagd/FlagdProvider.cs b/src/OpenFeature.Contrib.Providers.Flagd/FlagdProvider.cs
index 65e5c03..481a724 100644
--- a/src/OpenFeature.Contrib.Providers.Flagd/FlagdProvider.cs
+++ b/src/OpenFeature.Contrib.Providers.Flagd/FlagdProvider.cs
@@ -325,18 +325,10 @@ namespace OpenFeature.Contrib.Providers.Flagd
                         var response = call.ResponseStream.Current;

                         if (string.Equals(response.Type, "configuration_change", StringComparison.OrdinalIgnoreCase))
                         {
                             HandleConfigurationChangeEvent(response.Data);
-                            break;
                         }
-
-                        if (string.Equals(response.Type, "provider_ready", StringComparison.OrdinalIgnoreCase))
+                        else if (string.Equals(response.Type, "provider_ready", StringComparison.OrdinalIgnoreCase))
                         {
                             HandleProviderReadyEvent();
-                            break;
                         }
-
-                        break;
                     }
                 }
                 catch (RpcException ex) when (ex.StatusCode == StatusCode.Unavailable)

[toddbaert]

doh! mis-converted the original switch by retaining the break statements in the first place.

I noticed this about the same time you force pushed the fix. Understandable mistake 😅