Review feedback 2

NKnusperer · NKnusperer · commit b79a2028e80c · 2024-09-08T12:29:15.000+02:00
- Add `TypeAccessModifierOption` to client handlers.
- Add `TypeAccessModifier` to workspace `ApiClientConfiguration` and `ApiClientConfigurationComparer`.
- Add default value of `Public` for stringified `TypeAccessModifier` properties.
- Add test case for `CSharpRefiner`.
- Implement `IAccessibleElement` for `CodeClass` and `CodeEnum`.
diff --git a/src/Kiota.Builder/CodeDOM/CodeClass.cs b/src/Kiota.Builder/CodeDOM/CodeClass.cs
@@ -30,7 +30,7 @@ public enum CodeClassKind
 /// <summary>
 /// CodeClass represents an instance of a Class to be generated in source code
 /// </summary>
-public class CodeClass : ProprietableBlock<CodeClassKind, ClassDeclaration>, ITypeDefinition, IDiscriminatorInformationHolder, IDeprecableElement
+public class CodeClass : ProprietableBlock<CodeClassKind, ClassDeclaration>, ITypeDefinition, IDiscriminatorInformationHolder, IDeprecableElement, IAccessibleElement
 {
     private readonly ConcurrentDictionary<string, CodeProperty> PropertiesByWireName = new(StringComparer.OrdinalIgnoreCase);
     
diff --git a/src/Kiota.Builder/CodeDOM/CodeEnum.cs b/src/Kiota.Builder/CodeDOM/CodeEnum.cs
@@ -5,7 +5,7 @@
 
 namespace Kiota.Builder.CodeDOM;
 #pragma warning disable CA1711
-public class CodeEnum : CodeBlock<BlockDeclaration, BlockEnd>, IDocumentedElement, ITypeDefinition, IDeprecableElement
+public class CodeEnum : CodeBlock<BlockDeclaration, BlockEnd>, IDocumentedElement, ITypeDefinition, IDeprecableElement, IAccessibleElement
 {
 #pragma warning restore CA2227
     public AccessModifier Access { get; set; } = AccessModifier.Public;
diff --git a/src/Kiota.Builder/Lock/KiotaLock.cs b/src/Kiota.Builder/Lock/KiotaLock.cs
@@ -34,7 +34,7 @@ public class KiotaLock
     /// <summary>
     /// The type access modifier to use for the client types.
     /// </summary>
-    public string TypeAccessModifier { get; set; } = string.Empty;
+    public string TypeAccessModifier { get; set; } = "Public";
     /// <summary>
     /// The main namespace for this client.
     /// </summary>
diff --git a/src/Kiota.Builder/Refiners/CSharpRefiner.cs b/src/Kiota.Builder/Refiners/CSharpRefiner.cs
@@ -266,11 +266,9 @@ private void SetTypeAccessModifiers(CodeElement currentElement)
     {
         switch (currentElement)
         {
-            case CodeClass currentClass:
-                currentClass.Access = _configuration.TypeAccessModifier;
-                break;
-            case CodeEnum currentEnum:
-                currentEnum.Access = _configuration.TypeAccessModifier;
+            case CodeClass:
+            case CodeEnum:
+                ((IAccessibleElement)currentElement).Access = _configuration.TypeAccessModifier;
                 break;
         }
 
diff --git a/src/Kiota.Builder/WorkspaceManagement/ApiClientConfiguration.cs b/src/Kiota.Builder/WorkspaceManagement/ApiClientConfiguration.cs
@@ -2,6 +2,7 @@
 using System.Collections.Generic;
 using System.IO;
 using System.Linq;
+using Kiota.Builder.CodeDOM;
 using Kiota.Builder.Configuration;
 using Microsoft.OpenApi.ApiManifest;
 
@@ -15,6 +16,10 @@ public class ApiClientConfiguration : BaseApiConsumerConfiguration, ICloneable
     /// </summary>
     public string Language { get; set; } = string.Empty;
     /// <summary>
+    /// The type access modifier to use for the client types.
+    /// </summary>
+    public string TypeAccessModifier { get; set; } = "Public";
+    /// <summary>
     /// The structured mime types used for this client.
     /// </summary>
 #pragma warning disable CA1002
@@ -64,6 +69,7 @@ public ApiClientConfiguration(GenerationConfiguration config) : base(config)
     {
         ArgumentNullException.ThrowIfNull(config);
         Language = config.Language.ToString();
+        TypeAccessModifier = config.TypeAccessModifier.ToString();
         ClientNamespaceName = config.ClientNamespaceName;
         UsesBackingStore = config.UsesBackingStore;
         ExcludeBackwardCompatible = config.ExcludeBackwardCompatible;
@@ -84,6 +90,8 @@ public void UpdateGenerationConfigurationFromApiClientConfiguration(GenerationCo
         config.ClientNamespaceName = ClientNamespaceName;
         if (Enum.TryParse<GenerationLanguage>(Language, out var parsedLanguage))
             config.Language = parsedLanguage;
+        if (Enum.TryParse<AccessModifier>(TypeAccessModifier, out var parsedTypeAccessModifier))
+            config.TypeAccessModifier = parsedTypeAccessModifier;
         config.UsesBackingStore = UsesBackingStore;
         config.ExcludeBackwardCompatible = ExcludeBackwardCompatible;
         config.IncludeAdditionalData = IncludeAdditionalData;
@@ -97,6 +105,7 @@ public object Clone()
         var result = new ApiClientConfiguration
         {
             Language = Language,
+            TypeAccessModifier = TypeAccessModifier,
             StructuredMimeTypes = [.. StructuredMimeTypes],
             ClientNamespaceName = ClientNamespaceName,
             UsesBackingStore = UsesBackingStore,
diff --git a/src/Kiota.Builder/WorkspaceManagement/ApiClientConfigurationComparer.cs b/src/Kiota.Builder/WorkspaceManagement/ApiClientConfigurationComparer.cs
@@ -28,6 +28,7 @@ public override bool Equals(ApiClientConfiguration? x, ApiClientConfiguration? y
         if (x.IncludeAdditionalData != y.IncludeAdditionalData) return false;
         if (!_stringComparer.Equals(x.ClientNamespaceName, y.ClientNamespaceName)) return false;
         if (!_stringComparer.Equals(x.Language, y.Language)) return false;
+        if (!_stringComparer.Equals(x.TypeAccessModifier, y.TypeAccessModifier)) return false;
 
         // slow deep comparison
         return _stringIEnumerableDeepComparer.Equals(x.DisabledValidationRules, y.DisabledValidationRules)
@@ -42,6 +43,7 @@ public override int GetHashCode([DisallowNull] ApiClientConfiguration obj)
         hash.Add(obj.DisabledValidationRules, _stringIEnumerableDeepComparer); // _stringIEnumerableDeepComparer orders
         hash.Add(obj.ClientNamespaceName, _stringComparer);
         hash.Add(obj.Language, _stringComparer);
+        hash.Add(obj.TypeAccessModifier, _stringComparer);
         hash.Add(obj.ExcludeBackwardCompatible);
         hash.Add(obj.UsesBackingStore);
         hash.Add(obj.IncludeAdditionalData);
diff --git a/src/kiota/Handlers/Client/AddHandler.cs b/src/kiota/Handlers/Client/AddHandler.cs
@@ -2,6 +2,7 @@
 using System.CommandLine.Invocation;
 using System.Text.Json;
 using Kiota.Builder;
+using Kiota.Builder.CodeDOM;
 using Kiota.Builder.Configuration;
 using Kiota.Builder.Extensions;
 using Kiota.Builder.WorkspaceManagement;
@@ -27,6 +28,10 @@ public required Option<GenerationLanguage> LanguageOption
     {
         get; init;
     }
+    public required Option<AccessModifier> TypeAccessModifierOption
+    {
+        get; init;
+    }
     public required Option<string> DescriptionOption
     {
         get; init;
@@ -69,6 +74,7 @@ public override async Task<int> InvokeAsync(InvocationContext context)
     {
         string output = context.ParseResult.GetValueForOption(OutputOption) ?? string.Empty;
         GenerationLanguage language = context.ParseResult.GetValueForOption(LanguageOption);
+        AccessModifier typeAccessModifier = context.ParseResult.GetValueForOption(TypeAccessModifierOption);
         string openapi = context.ParseResult.GetValueForOption(DescriptionOption) ?? string.Empty;
         bool backingStore = context.ParseResult.GetValueForOption(BackingStoreOption);
         bool excludeBackwardCompatible = context.ParseResult.GetValueForOption(ExcludeBackwardCompatibleOption);
@@ -90,6 +96,7 @@ public override async Task<int> InvokeAsync(InvocationContext context)
         Configuration.Generation.IncludeAdditionalData = includeAdditionalData;
         Configuration.Generation.Language = language;
         WarnUsingPreviewLanguage(language);
+        Configuration.Generation.TypeAccessModifier = typeAccessModifier;
         Configuration.Generation.SkipGeneration = skipGeneration;
         Configuration.Generation.Operation = ConsumerOperation.Add;
         if (includePatterns.Count != 0)
diff --git a/src/kiota/Handlers/Client/EditHandler.cs b/src/kiota/Handlers/Client/EditHandler.cs
@@ -2,6 +2,7 @@
 using System.CommandLine.Invocation;
 using System.Text.Json;
 using Kiota.Builder;
+using Kiota.Builder.CodeDOM;
 using Kiota.Builder.Configuration;
 using Kiota.Builder.Extensions;
 using Kiota.Builder.WorkspaceManagement;
@@ -27,6 +28,10 @@ public required Option<GenerationLanguage?> LanguageOption
     {
         get; init;
     }
+    public required Option<AccessModifier?> TypeAccessModifierOption
+    {
+        get; init;
+    }
     public required Option<string> DescriptionOption
     {
         get; init;
@@ -69,6 +74,7 @@ public override async Task<int> InvokeAsync(InvocationContext context)
     {
         string output = context.ParseResult.GetValueForOption(OutputOption) ?? string.Empty;
         GenerationLanguage? language = context.ParseResult.GetValueForOption(LanguageOption);
+        AccessModifier? typeAccessModifier = context.ParseResult.GetValueForOption(TypeAccessModifierOption);
         string openapi = context.ParseResult.GetValueForOption(DescriptionOption) ?? string.Empty;
         bool? backingStore = context.ParseResult.GetValueForOption(BackingStoreOption);
         bool? excludeBackwardCompatible = context.ParseResult.GetValueForOption(ExcludeBackwardCompatibleOption);
@@ -109,6 +115,8 @@ public override async Task<int> InvokeAsync(InvocationContext context)
                 clientConfiguration.UpdateGenerationConfigurationFromApiClientConfiguration(Configuration.Generation, className);
                 if (language.HasValue)
                     Configuration.Generation.Language = language.Value;
+                if (typeAccessModifier.HasValue)
+                    Configuration.Generation.TypeAccessModifier = typeAccessModifier.Value;
                 if (backingStore.HasValue)
                     Configuration.Generation.UsesBackingStore = backingStore.Value;
                 if (excludeBackwardCompatible.HasValue)
diff --git a/src/kiota/KiotaClientCommands.cs b/src/kiota/KiotaClientCommands.cs
@@ -33,6 +33,7 @@ public static Command GetAddCommand()
     {
         var defaultConfiguration = new GenerationConfiguration();
         var languageOption = KiotaHost.GetLanguageOption();
+        var typeAccessModifierOption = KiotaHost.GetTypeAccessModifierOption();
         var outputOption = KiotaHost.GetOutputPathOption(defaultConfiguration.OutputPath);
         var descriptionOption = KiotaHost.GetDescriptionOption(defaultConfiguration.OpenAPIFilePath, true);
         var namespaceOption = KiotaHost.GetNamespaceOption(defaultConfiguration.ClientNamespaceName);
@@ -50,6 +51,7 @@ public static Command GetAddCommand()
             descriptionOption,
             outputOption,
             languageOption,
+            typeAccessModifierOption,
             clientNameOption,
             namespaceOption,
             logLevelOption,
@@ -67,6 +69,7 @@ public static Command GetAddCommand()
             DescriptionOption = descriptionOption,
             OutputOption = outputOption,
             LanguageOption = languageOption,
+            TypeAccessModifierOption = typeAccessModifierOption,
             ClassOption = clientNameOption,
             NamespaceOption = namespaceOption,
             LogLevelOption = logLevelOption,
@@ -104,6 +107,7 @@ public static Command GetRemoveCommand()
     public static Command GetEditCommand()
     {
         var languageOption = KiotaHost.GetOptionalLanguageOption();
+        var typeAccessModifierOption = KiotaHost.GetOptionalTypeAccessModifierOption();
         var outputOption = KiotaHost.GetOutputPathOption(string.Empty);
         var descriptionOption = KiotaHost.GetDescriptionOption(string.Empty);
         var namespaceOption = KiotaHost.GetNamespaceOption(string.Empty);
@@ -121,6 +125,7 @@ public static Command GetEditCommand()
             descriptionOption,
             outputOption,
             languageOption,
+            typeAccessModifierOption,
             clientNameOption,
             namespaceOption,
             logLevelOption,
@@ -138,6 +143,7 @@ public static Command GetEditCommand()
             DescriptionOption = descriptionOption,
             OutputOption = outputOption,
             LanguageOption = languageOption,
+            TypeAccessModifierOption = typeAccessModifierOption,
             ClassOption = clientNameOption,
             NamespaceOption = namespaceOption,
             LogLevelOption = logLevelOption,
diff --git a/src/kiota/KiotaHost.cs b/src/kiota/KiotaHost.cs
@@ -343,19 +343,26 @@ internal static Option<GenerationLanguage> GetLanguageOption()
         AddEnumValidator(languageOption, "language");
         return languageOption;
     }
+    internal static Option<GenerationLanguage?> GetOptionalLanguageOption()
+    {
+        var languageOption = new Option<GenerationLanguage?>("--language", "The target language for the generated code files.");
+        languageOption.AddAlias("-l");
+        AddEnumValidator(languageOption, "language");
+        return languageOption;
+    }
     internal static Option<AccessModifier> GetTypeAccessModifierOption()
     {
         var accessOption = new Option<AccessModifier>("--type-access-modifier", "The type access modifier to use for the client types.");
         accessOption.AddAlias("--tam");
         AddEnumValidator(accessOption, "type-access-modifier");
         return accessOption;
     }
-    internal static Option<GenerationLanguage?> GetOptionalLanguageOption()
+    internal static Option<AccessModifier?> GetOptionalTypeAccessModifierOption()
     {
-        var languageOption = new Option<GenerationLanguage?>("--language", "The target language for the generated code files.");
-        languageOption.AddAlias("-l");
-        AddEnumValidator(languageOption, "language");
-        return languageOption;
+        var accessOption = new Option<AccessModifier?>("--type-access-modifier", "The type access modifier to use for the client types.");
+        accessOption.AddAlias("--tam");
+        AddEnumValidator(accessOption, "type-access-modifier");
+        return accessOption;
     }
     internal static Option<string> GetNamespaceOption(string defaultNamespaceName)
     {
diff --git a/tests/Kiota.Builder.Tests/Refiners/CSharpLanguageRefinerTests.cs b/tests/Kiota.Builder.Tests/Refiners/CSharpLanguageRefinerTests.cs
@@ -910,5 +910,23 @@ public async Task AddsUsingForUntypedNodeAsync()
         Assert.Equal("Microsoft.Kiota.Abstractions.Serialization", nodeUsing[0].Declaration.Name);
 
     }
+    [Theory]
+    [InlineData(AccessModifier.Public)]
+    [InlineData(AccessModifier.Internal)]
+    public async Task SetTypeAccessModifierAsync(AccessModifier accessModifier)
+    {
+        var codeClass = root.AddClass(new CodeClass
+        {
+            Name = "Class1",
+            Kind = CodeClassKind.Model
+        }).First();
+        var codeEnum = root.AddEnum(new CodeEnum
+        {
+            Name = "Enum1",
+        }).First();
+        await ILanguageRefiner.RefineAsync(new GenerationConfiguration { Language = GenerationLanguage.CSharp, TypeAccessModifier = accessModifier}, root);
+        Assert.Equal(codeClass.Access, accessModifier);
+        Assert.Equal(codeEnum.Access, accessModifier);
+    }
     #endregion
 }

Original file line number	Diff line number	Diff line change
`@@ -30,7 +30,7 @@ public enum CodeClassKind`
`30`	`30`	`/// <summary>`
`31`	`31`	`/// CodeClass represents an instance of a Class to be generated in source code`
`32`	`32`	`/// </summary>`
`33`		`-public class CodeClass : ProprietableBlock<CodeClassKind, ClassDeclaration>, ITypeDefinition, IDiscriminatorInformationHolder, IDeprecableElement`
	`33`	`+public class CodeClass : ProprietableBlock<CodeClassKind, ClassDeclaration>, ITypeDefinition, IDiscriminatorInformationHolder, IDeprecableElement, IAccessibleElement`
`34`	`34`	`{`
`35`	`35`	`private readonly ConcurrentDictionary<string, CodeProperty> PropertiesByWireName = new(StringComparer.OrdinalIgnoreCase);`
`36`	`36`
Original file line number	Diff line number	Diff line change
`@@ -5,7 +5,7 @@`
`5`	`5`
`6`	`6`	`namespace Kiota.Builder.CodeDOM;`
`7`	`7`	`#pragma warning disable CA1711`
`8`		`-public class CodeEnum : CodeBlock<BlockDeclaration, BlockEnd>, IDocumentedElement, ITypeDefinition, IDeprecableElement`
	`8`	`+public class CodeEnum : CodeBlock<BlockDeclaration, BlockEnd>, IDocumentedElement, ITypeDefinition, IDeprecableElement, IAccessibleElement`
`9`	`9`	`{`
`10`	`10`	`#pragma warning restore CA2227`
`11`	`11`	`public AccessModifier Access { get; set; } = AccessModifier.Public;`
Original file line number	Diff line number	Diff line change
`@@ -266,11 +266,9 @@ private void SetTypeAccessModifiers(CodeElement currentElement)`
`266`	`266`	`{`
`267`	`267`	`switch (currentElement)`
`268`	`268`	`{`
`269`		`- case CodeClass currentClass:`
`270`		`- currentClass.Access = _configuration.TypeAccessModifier;`
`271`		`- break;`
`272`		`- case CodeEnum currentEnum:`
`273`		`- currentEnum.Access = _configuration.TypeAccessModifier;`
	`269`	`+ case CodeClass:`
	`270`	`+ case CodeEnum:`
	`271`	`+ ((IAccessibleElement)currentElement).Access = _configuration.TypeAccessModifier;`
`274`	`272`	`break;`
`275`	`273`	`}`
`276`	`274`
Original file line number	Diff line number	Diff line change
`@@ -343,19 +343,26 @@ internal static Option<GenerationLanguage> GetLanguageOption()`
`343`	`343`	`AddEnumValidator(languageOption, "language");`
`344`	`344`	`return languageOption;`
`345`	`345`	`}`
	`346`	`+ internal static Option<GenerationLanguage?> GetOptionalLanguageOption()`
	`347`	`+ {`
	`348`	`+ var languageOption = new Option<GenerationLanguage?>("--language", "The target language for the generated code files.");`
	`349`	`+ languageOption.AddAlias("-l");`
	`350`	`+ AddEnumValidator(languageOption, "language");`
	`351`	`+ return languageOption;`
	`352`	`+ }`
`346`	`353`	`internal static Option<AccessModifier> GetTypeAccessModifierOption()`
`347`	`354`	`{`
`348`	`355`	`var accessOption = new Option<AccessModifier>("--type-access-modifier", "The type access modifier to use for the client types.");`
`349`	`356`	`accessOption.AddAlias("--tam");`
`350`	`357`	`AddEnumValidator(accessOption, "type-access-modifier");`
`351`	`358`	`return accessOption;`
`352`	`359`	`}`
`353`		`- internal static Option<GenerationLanguage?> GetOptionalLanguageOption()`
	`360`	`+ internal static Option<AccessModifier?> GetOptionalTypeAccessModifierOption()`
`354`	`361`	`{`
`355`		`- var languageOption = new Option<GenerationLanguage?>("--language", "The target language for the generated code files.");`
`356`		`- languageOption.AddAlias("-l");`
`357`		`- AddEnumValidator(languageOption, "language");`
`358`		`- return languageOption;`
	`362`	`+ var accessOption = new Option<AccessModifier?>("--type-access-modifier", "The type access modifier to use for the client types.");`
	`363`	`+ accessOption.AddAlias("--tam");`
	`364`	`+ AddEnumValidator(accessOption, "type-access-modifier");`
	`365`	`+ return accessOption;`
`359`	`366`	`}`
`360`	`367`	`internal static Option<string> GetNamespaceOption(string defaultNamespaceName)`
`361`	`368`	`{`