tryAGI / AutoSDK

Automated .NET SDKs for your APIs
https://autosdk.net
Other
15 stars 2 forks source link

handle properties that have non-c# valid names #14

Closed Tyler-IN closed 4 months ago

Tyler-IN commented 4 months ago

Follow up issue after https://github.com/HavenDV/OpenApiGenerator/issues/12

Part of the spec I am working with has aspect ratios as property names as well, such as '1.5' and '16:9'.

Need to generalize the property name sanitization.

HavenDV commented 4 months ago

image https://learn.microsoft.com/en-us/dotnet/csharp/fundamentals/coding-style/identifier-names#naming-rules

HavenDV commented 4 months ago

Yes, I have so far taken the path of fixing single problems instead of a general universal solution. This is what the search gave me

HavenDV commented 4 months ago

The problem is that some characters can be counted as separators between words, and some need to be replaced with something (as in your case). And itโ€™s difficult to determine a universal rule without knowing unique cases

Tyler-IN commented 4 months ago

No arguments there. Once you get down to defining property data, you have covered most other scenarios.

Here's my proposal,

@ src/libs/OpenApiGenerator.Core/Models/PropertyData.cs:52
-        if (name[0] is not ('_' or >= 'A' and <= 'Z' or >= 'a' and <= 'z'))
-        {
-            name = $"_{name}";
-        }
- 
         if (parents.Length != 0)
         {
             name = name.FixPropertyName(parents.Last().ClassName);
         }
+ 
+        SanitizeName(ref name);
    private static void SanitizeName(ref string name)
    {
        static bool InvalidFirstChar(char ch)
            => ch is not ('_' or >= 'A' and <= 'Z' or >= 'a' and <= 'z');

        static bool InvalidSubsequentChar(char ch)
            => ch is not (
                    '_'
                    or >= 'A' and <= 'Z'
                    or >= 'a' and <= 'z'
                    or >= '0' and <= '9'
                );

        if (InvalidFirstChar(name[0]))
        {
            name = $"_{name}";
        }

        if (!name.Skip(1).Any(InvalidSubsequentChar)) return;

        Span<char> buf = stackalloc char[name.Length];
        name.AsSpan().CopyTo(buf);

        for (var i = 1; i < buf.Length; i++)
        {
            if (InvalidSubsequentChar(buf[i]))
            {
                buf[i] = '_';
            }
        }

        // Span<T>.ToString implementation checks for char type, new string(&buf[0], buf.length)
        name = buf.ToString();
    }

Made a few revisions to the above code, can see the edits. ๐Ÿ˜„

The validation functions InvalidFirstChar and InvalidSubsequentChar can be extended to handle additional cases (such as Unicode support) as needed.

This implementation does not address multi-character code points and could be generalized to use a formal UTF-32 transform buffer and validate code points by what the C# team has referred to as 'Runes'.

HavenDV commented 4 months ago

I explored some valid properties: image But Iโ€™m not sure that this can be at least somehow useful and that it should be used. Your solution is good, but I don't know how to combine it with division for words and automatic translation of each word into uppercase

HavenDV commented 4 months ago

Please also check my latest commit, I added separate tests for special cases and simple fix for your case, but I will think how to apply your latest PR https://github.com/HavenDV/OpenApiGenerator/commit/95c2372a6feafdc91c951911cb922abab528c069

Tyler-IN commented 4 months ago

Hopefully this covers all of my use cases.

I think extending it to support these cases can be done with some effort.

I hope I got all of the changes rebased in.

Looking forward to using this project!

HavenDV commented 4 months ago

I released 0.8.5 with your latest changes, should be available soon

Tyler-IN commented 4 months ago

dotnet/roslyn:src/Compilers/Core/Portable/InternalUtilities/UnicodeCharacterUtilities.cs outlines what makes valid property names.

Can maybe use that for reference when expanding SanitizeNames, etc.

Tyler-IN commented 4 months ago

This solves my use case, but noticed something else when adding the Twitch API... Well... I'll create a new issue...

Tyler-IN commented 4 months ago

๐Ÿ‘