chore: cleanup code (open-feature#277)

<!-- Please use this template for your pull request. -->
<!-- Please use the sections that you need and delete other sections -->

## This PR
This PR fixes some of the warnings and typos that I recently found.
More interestingly, it addresses these issues:
- Missing the `.this`
- Usage of `ILogger` vs `Source generator log`
- `const` vs `static`
- Fix nullability for some methods and properties.
And a few more changes.

### Follow-up Tasks
We need to do more cleanup tasks.

### How to test
All of these changes are recommended by the IDE and "tested" by the
compiler when it executes.

---------

Signed-off-by: André Silva <[email protected]>
Signed-off-by: Artyom Tonoyan <[email protected]>

Author: askpt

src/OpenFeature/Api.cs

@@ -32,7 +32,7 @@ public sealed class Api : IEventBus
         public static Api Instance { get; } = new Api();
 
         // Explicit static constructor to tell C# compiler
-        // not to mark type as beforefieldinit
+        // not to mark type as beforeFieldInit
         // IE Lazy way of ensuring this is thread safe without using locks
         static Api() { }
         private Api() { }
@@ -46,7 +46,7 @@ private Api() { }
         public async Task SetProviderAsync(FeatureProvider featureProvider)
         {
             this._eventExecutor.RegisterDefaultFeatureProvider(featureProvider);
-            await this._repository.SetProviderAsync(featureProvider, this.GetContext(), AfterInitialization, AfterError).ConfigureAwait(false);
+            await this._repository.SetProviderAsync(featureProvider, this.GetContext(), this.AfterInitialization, this.AfterError).ConfigureAwait(false);
         }
 
         /// <summary>
@@ -62,7 +62,7 @@ public async Task SetProviderAsync(string clientName, FeatureProvider featurePro
                 throw new ArgumentNullException(nameof(clientName));
             }
             this._eventExecutor.RegisterClientFeatureProvider(clientName, featureProvider);
-            await this._repository.SetProviderAsync(clientName, featureProvider, this.GetContext(), AfterInitialization, AfterError).ConfigureAwait(false);
+            await this._repository.SetProviderAsync(clientName, featureProvider, this.GetContext(), this.AfterInitialization, this.AfterError).ConfigureAwait(false);
         }
 
         /// <summary>
@@ -101,15 +101,15 @@ public FeatureProvider GetProvider(string clientName)
         /// </para>
         /// </summary>
         /// <returns><see cref="ClientMetadata"/></returns>
-        public Metadata GetProviderMetadata() => this.GetProvider().GetMetadata();
+        public Metadata? GetProviderMetadata() => this.GetProvider().GetMetadata();
 
         /// <summary>
         /// Gets providers metadata assigned to the given clientName. If the clientName has no provider
         /// assigned to it the default provider will be returned
         /// </summary>
         /// <param name="clientName">Name of client</param>
         /// <returns>Metadata assigned to provider</returns>
-        public Metadata GetProviderMetadata(string clientName) => this.GetProvider(clientName).GetMetadata();
+        public Metadata? GetProviderMetadata(string clientName) => this.GetProvider(clientName).GetMetadata();
 
         /// <summary>
         /// Create a new instance of <see cref="FeatureClient"/> using the current provider
@@ -121,7 +121,7 @@ public FeatureProvider GetProvider(string clientName)
         /// <returns><see cref="FeatureClient"/></returns>
         public FeatureClient GetClient(string? name = null, string? version = null, ILogger? logger = null,
             EvaluationContext? context = null) =>
-            new FeatureClient(() => _repository.GetProvider(name), name, version, logger, context);
+            new FeatureClient(() => this._repository.GetProvider(name), name, version, logger, context);
 
         /// <summary>
         /// Appends list of hooks to global hooks list
@@ -277,7 +277,7 @@ private async Task AfterInitialization(FeatureProvider provider)
             {
                 Type = ProviderEventTypes.ProviderReady,
                 Message = "Provider initialization complete",
-                ProviderName = provider.GetMetadata().Name,
+                ProviderName = provider.GetMetadata()?.Name,
             };
 
             await this._eventExecutor.EventChannel.Writer.WriteAsync(new Event { Provider = provider, EventPayload = eventPayload }).ConfigureAwait(false);
@@ -286,10 +286,9 @@ private async Task AfterInitialization(FeatureProvider provider)
         /// <summary>
         /// Update the provider state to ERROR and emit an ERROR after failed init.
         /// </summary>
-        private async Task AfterError(FeatureProvider provider, Exception ex)
-
+        private async Task AfterError(FeatureProvider provider, Exception? ex)
         {
-            provider.Status = typeof(ProviderFatalException) == ex.GetType() ? ProviderStatus.Fatal : ProviderStatus.Error;
+            provider.Status = typeof(ProviderFatalException) == ex?.GetType() ? ProviderStatus.Fatal : ProviderStatus.Error;
             var eventPayload = new ProviderEventPayload
             {
                 Type = ProviderEventTypes.ProviderError,

src/OpenFeature/Constant/Reason.cs

@@ -9,42 +9,42 @@ public static class Reason
         /// <summary>
         /// Use when the flag is matched based on the evaluation context user data
         /// </summary>
-        public static string TargetingMatch = "TARGETING_MATCH";
+        public const string TargetingMatch = "TARGETING_MATCH";
 
         /// <summary>
         /// Use when the flag is matched based on a split rule in the feature flag provider
         /// </summary>
-        public static string Split = "SPLIT";
+        public const string Split = "SPLIT";
 
         /// <summary>
         /// Use when the flag is disabled in the feature flag provider
         /// </summary>
-        public static string Disabled = "DISABLED";
+        public const string Disabled = "DISABLED";
 
         /// <summary>
         /// Default reason when evaluating flag
         /// </summary>
-        public static string Default = "DEFAULT";
+        public const string Default = "DEFAULT";
 
         /// <summary>
         /// The resolved value is static (no dynamic evaluation)
         /// </summary>
-        public static string Static = "STATIC";
+        public const string Static = "STATIC";
 
         /// <summary>
         /// The resolved value was retrieved from cache
         /// </summary>
-        public static string Cached = "CACHED";
+        public const string Cached = "CACHED";
 
         /// <summary>
         /// Use when an unknown reason is encountered when evaluating flag.
         /// An example of this is if the feature provider returns a reason that is not defined in the spec
         /// </summary>
-        public static string Unknown = "UNKNOWN";
+        public const string Unknown = "UNKNOWN";
 
         /// <summary>
         /// Use this flag when abnormal execution is encountered.
         /// </summary>
-        public static string Error = "ERROR";
+        public const string Error = "ERROR";
     }
 }

src/OpenFeature/Error/ProviderFatalException.cs

@@ -4,7 +4,7 @@
 
 namespace OpenFeature.Error
 {
-    /// <summary> the
+    /// <summary>
     /// An exception that signals the provider has entered an irrecoverable error state.
     /// </summary>
     [ExcludeFromCodeCoverage]

src/OpenFeature/EventExecutor.cs

@@ -206,7 +206,7 @@ private void EmitOnRegistration(FeatureProvider? provider, ProviderEventTypes ev
                 {
                     handler.Invoke(new ProviderEventPayload
                     {
-                        ProviderName = provider.GetMetadata().Name,
+                        ProviderName = provider.GetMetadata()?.Name,
                         Type = eventType,
                         Message = message
                     });
@@ -322,6 +322,7 @@ private void UpdateProviderStatus(FeatureProvider provider, ProviderEventPayload
                 case ProviderEventTypes.ProviderError:
                     provider.Status = eventPayload.ErrorType == ErrorType.ProviderFatal ? ProviderStatus.Fatal : ProviderStatus.Error;
                     break;
+                case ProviderEventTypes.ProviderConfigurationChanged:
                 default: break;
             }
         }

src/OpenFeature/FeatureProvider.cs

@@ -11,14 +11,14 @@ namespace OpenFeature
 {
     /// <summary>
     /// The provider interface describes the abstraction layer for a feature flag provider.
-    /// A provider acts as the translates layer between the generic feature flag structure to a target feature flag system.
+    /// A provider acts as it translates layer between the generic feature flag structure to a target feature flag system.
     /// </summary>
     /// <seealso href="https://github.com/open-feature/spec/blob/v0.5.2/specification/sections/02-providers.md">Provider specification</seealso>
     public abstract class FeatureProvider
     {
         /// <summary>
-        /// Gets a immutable list of hooks that belong to the provider.
-        /// By default return a empty list
+        /// Gets an immutable list of hooks that belong to the provider.
+        /// By default, return an empty list
         ///
         /// Executed in the order of hooks
         /// before: API, Client, Invocation, Provider
@@ -38,7 +38,7 @@ public abstract class FeatureProvider
         /// Metadata describing the provider.
         /// </summary>
         /// <returns><see cref="Metadata"/></returns>
-        public abstract Metadata GetMetadata();
+        public abstract Metadata? GetMetadata();
 
         /// <summary>
         /// Resolves a boolean feature flag

src/OpenFeature/Model/EvaluationContextBuilder.cs

@@ -140,9 +140,9 @@ public EvaluationContextBuilder Merge(EvaluationContext context)
         {
             string? newTargetingKey = "";
 
-            if (!string.IsNullOrWhiteSpace(TargetingKey))
+            if (!string.IsNullOrWhiteSpace(this.TargetingKey))
             {
-                newTargetingKey = TargetingKey;
+                newTargetingKey = this.TargetingKey;
             }
 
             if (!string.IsNullOrWhiteSpace(context.TargetingKey))

src/OpenFeature/Model/FlagEvaluationOptions.cs

@@ -10,12 +10,12 @@ namespace OpenFeature.Model
     public sealed class FlagEvaluationOptions
     {
         /// <summary>
-        /// A immutable list of <see cref="Hook"/>
+        /// An immutable list of <see cref="Hook"/>
         /// </summary>
         public IImmutableList<Hook> Hooks { get; }
 
         /// <summary>
-        /// A immutable dictionary of hook hints
+        /// An immutable dictionary of hook hints
         /// </summary>
         public IImmutableDictionary<string, object> HookHints { get; }
 

src/OpenFeature/Model/ImmutableMetadata.cs

@@ -1,7 +1,6 @@
 using System.Collections.Generic;
 using System.Collections.Immutable;
 
-#nullable enable
 namespace OpenFeature.Model;
 
 /// <summary>

src/OpenFeature/Model/Value.cs

@@ -139,49 +139,49 @@ public Value(Object value)
         public object? AsObject => this._innerValue;
 
         /// <summary>
-        /// Returns the underlying int value
-        /// Value will be null if it isn't a integer
+        /// Returns the underlying int value.
+        /// Value will be null if it isn't an integer
         /// </summary>
         /// <returns>Value as int</returns>
-        public int? AsInteger => this.IsNumber ? (int?)Convert.ToInt32((double?)this._innerValue) : null;
+        public int? AsInteger => this.IsNumber ? Convert.ToInt32((double?)this._innerValue) : null;
 
         /// <summary>
-        /// Returns the underlying bool value
+        /// Returns the underlying bool value.
         /// Value will be null if it isn't a bool
         /// </summary>
         /// <returns>Value as bool</returns>
         public bool? AsBoolean => this.IsBoolean ? (bool?)this._innerValue : null;
 
         /// <summary>
-        /// Returns the underlying double value
+        /// Returns the underlying double value.
         /// Value will be null if it isn't a double
         /// </summary>
         /// <returns>Value as int</returns>
         public double? AsDouble => this.IsNumber ? (double?)this._innerValue : null;
 
         /// <summary>
-        /// Returns the underlying string value
+        /// Returns the underlying string value.
         /// Value will be null if it isn't a string
         /// </summary>
         /// <returns>Value as string</returns>
         public string? AsString => this.IsString ? (string?)this._innerValue : null;
 
         /// <summary>
-        /// Returns the underlying Structure value
+        /// Returns the underlying Structure value.
         /// Value will be null if it isn't a Structure
         /// </summary>
         /// <returns>Value as Structure</returns>
         public Structure? AsStructure => this.IsStructure ? (Structure?)this._innerValue : null;
 
         /// <summary>
-        /// Returns the underlying List value
+        /// Returns the underlying List value.
         /// Value will be null if it isn't a List
         /// </summary>
         /// <returns>Value as List</returns>
         public IImmutableList<Value>? AsList => this.IsList ? (IImmutableList<Value>?)this._innerValue : null;
 
         /// <summary>
-        /// Returns the underlying DateTime value
+        /// Returns the underlying DateTime value.
         /// Value will be null if it isn't a DateTime
         /// </summary>
         /// <returns>Value as DateTime</returns>

src/OpenFeature/OpenFeatureClient.cs

@@ -212,11 +212,8 @@ private async Task<FlagEvaluationDetails<T>> EvaluateFlagAsync<T>(
             var resolveValueDelegate = providerInfo.Item1;
             var provider = providerInfo.Item2;
 
-            // New up a evaluation context if one was not provided.
-            if (context == null)
-            {
-                context = EvaluationContext.Empty;
-            }
+            // New up an evaluation context if one was not provided.
+            context ??= EvaluationContext.Empty;
 
             // merge api, client, and invocation context.
             var evaluationContext = Api.Instance.GetContext();
@@ -253,11 +250,11 @@ private async Task<FlagEvaluationDetails<T>> EvaluateFlagAsync<T>(
                 var contextFromHooks = await this.TriggerBeforeHooksAsync(allHooks, hookContext, options, cancellationToken).ConfigureAwait(false);
 
                 // short circuit evaluation entirely if provider is in a bad state
-                if (provider.Status == ProviderStatus.NotReady)
+                if (provider.Status == ProviderStatus.NotReady)
                 {
                     throw new ProviderNotReadyException("Provider has not yet completed initialization.");
-                }
-                else if (provider.Status == ProviderStatus.Fatal)
+                }
+                else if (provider.Status == ProviderStatus.Fatal)
                 {
                     throw new ProviderFatalException("Provider is in an irrecoverable error state.");
                 }
@@ -349,7 +346,7 @@ private async Task TriggerFinallyHooksAsync<T>(IReadOnlyList<Hook> hooks, HookCo
                 }
                 catch (Exception e)
                 {
-                    this._logger.LogError(e, "Error while executing Finally hook {HookName}", hook.GetType().Name);
+                    this.FinallyHookError(hook.GetType().Name, e);
                 }
             }
         }

src/OpenFeature/ProviderRepository.cs

@@ -14,9 +14,9 @@ namespace OpenFeature
     /// <summary>
     /// This class manages the collection of providers, both default and named, contained by the API.
     /// </summary>
-    internal sealed class ProviderRepository : IAsyncDisposable
+    internal sealed partial class ProviderRepository : IAsyncDisposable
     {
-        private ILogger _logger;
+        private ILogger _logger = NullLogger<EventExecutor>.Instance;
 
         private FeatureProvider _defaultProvider = new NoOpFeatureProvider();
 
@@ -26,20 +26,15 @@ internal sealed class ProviderRepository : IAsyncDisposable
         /// The reader/writer locks is not disposed because the singleton instance should never be disposed.
         ///
         /// Mutations of the _defaultProvider or _featureProviders are done within this lock even though
-        /// _featureProvider is a concurrent collection. This is for a couple reasons, the first is that
+        /// _featureProvider is a concurrent collection. This is for a couple of reasons, the first is that
         /// a provider should only be shutdown if it is not in use, and it could be in use as either a named or
         /// default provider.
         ///
-        /// The second is that a concurrent collection doesn't provide any ordering so we could check a provider
+        /// The second is that a concurrent collection doesn't provide any ordering, so we could check a provider
         /// as it was being added or removed such as two concurrent calls to SetProvider replacing multiple instances
-        /// of that provider under different names..
+        /// of that provider under different names.
         private readonly ReaderWriterLockSlim _providersLock = new ReaderWriterLockSlim();
 
-        public ProviderRepository()
-        {
-            this._logger = NullLogger<EventExecutor>.Instance;
-        }
-
         public async ValueTask DisposeAsync()
         {
             using (this._providersLock)
@@ -201,15 +196,15 @@ private async Task ShutdownIfUnusedAsync(
                 return;
             }
 
-            await SafeShutdownProviderAsync(targetProvider).ConfigureAwait(false);
+            await this.SafeShutdownProviderAsync(targetProvider).ConfigureAwait(false);
         }
 
         /// <remarks>
         /// <para>
         /// Shut down the provider and capture any exceptions thrown.
         /// </para>
         /// <para>
-        /// The provider is set either to a name or default before the old provider it shutdown, so
+        /// The provider is set either to a name or default before the old provider it shut down, so
         /// it would not be meaningful to emit an error.
         /// </para>
         /// </remarks>
@@ -226,7 +221,7 @@ private async Task SafeShutdownProviderAsync(FeatureProvider? targetProvider)
             }
             catch (Exception ex)
             {
-                this._logger.LogError(ex, $"Error shutting down provider: {targetProvider.GetMetadata().Name}");
+                this.ErrorShuttingDownProvider(targetProvider.GetMetadata()?.Name, ex);
             }
         }
 
@@ -287,8 +282,11 @@ public async Task ShutdownAsync(Action<FeatureProvider, Exception>? afterError =
             foreach (var targetProvider in providers)
             {
                 // We don't need to take any actions after shutdown.
-                await SafeShutdownProviderAsync(targetProvider).ConfigureAwait(false);
+                await this.SafeShutdownProviderAsync(targetProvider).ConfigureAwait(false);
             }
         }
+
+        [LoggerMessage(EventId = 105, Level = LogLevel.Error, Message = "Error shutting down provider: {TargetProviderName}`")]
+        partial void ErrorShuttingDownProvider(string? targetProviderName, Exception exception);
     }
 }