Tweaks to `String.safeForSwiftCode` to inform caller if a change was made

[theoriginalbit]

While merging main into my PR, #628, I noticed the changes introduced by #638 and #640 and it made me think more about the TranslatorContext and how it could provide more context (😄) to my translateVariableCase(_:) function. Specifically the if statement which needs to compare the two strings to see if the swiftSafeName function actually changed the output to determine if a raw value is required.

My thinking around this is that the String.safeForSwiftCode function already has the necessary information to determine if it made changes, without needing to perform string comparisons.

Knowing how many places use the closure on the context, I think a purely additive (opt-in) change should be utilised to allow any code that is interested in if there was a change to call an alternative closure. A very quick test I did for this looked like:

diff --git a/Sources/_OpenAPIGeneratorCore/Translator/CommonTranslations/SwiftSafeNames.swift b/Sources/_OpenAPIGeneratorCore/Translator/CommonTranslations/SwiftSafeNames.swift
index 9a52bc0..76b2b25 100644
--- a/Sources/_OpenAPIGeneratorCore/Translator/CommonTranslations/SwiftSafeNames.swift
+++ b/Sources/_OpenAPIGeneratorCore/Translator/CommonTranslations/SwiftSafeNames.swift
@@ -26,13 +26,14 @@ extension String {
     ///
     /// In addition to replacing illegal characters, it also
     /// ensures that the identifier starts with a letter and not a number.
-    var safeForSwiftCode: String {
-        guard !isEmpty else { return "_empty" }
+    var safeForSwiftCode: (String, requiredChanges: Bool) {
+        guard !isEmpty else { return ("_empty", requiredChanges: true) }
 
         let firstCharSet: CharacterSet = .letters.union(.init(charactersIn: "_"))
         let numbers: CharacterSet = .decimalDigits
         let otherCharSet: CharacterSet = .alphanumerics.union(.init(charactersIn: "_"))
 
+        var changed = false
         var sanitizedScalars: [Unicode.Scalar] = []
         for (index, scalar) in unicodeScalars.enumerated() {
             let allowedSet = index == 0 ? firstCharSet : otherCharSet
@@ -40,9 +41,11 @@ extension String {
             if allowedSet.contains(scalar) {
                 outScalar = scalar
             } else if index == 0 && numbers.contains(scalar) {
+                changed = true
                 sanitizedScalars.append("_")
                 outScalar = scalar
             } else {
+                changed = true
                 sanitizedScalars.append("_")
                 if let entityName = Self.specialCharsMap[scalar] {
                     for char in entityName.unicodeScalars { sanitizedScalars.append(char) }
@@ -61,10 +64,10 @@ extension String {
 
         //Special case for a single underscore.
         //We can't add it to the map as its a valid swift identifier in other cases.
-        if validString == "_" { return "_underscore_" }
+        if validString == "_" { return ("_underscore_", requiredChanges: true) }
 
-        guard Self.keywords.contains(validString) else { return validString }
-        return "_\(validString)"
+        guard Self.keywords.contains(validString) else { return (validString, requiredChanges: changed) }
+        return ("_\(validString)", requiredChanges: true)
     }
 
     /// A list of Swift keywords.
diff --git a/Sources/_OpenAPIGeneratorCore/Translator/FileTranslator.swift b/Sources/_OpenAPIGeneratorCore/Translator/FileTranslator.swift
index 4f24652..3d69dad 100644
--- a/Sources/_OpenAPIGeneratorCore/Translator/FileTranslator.swift
+++ b/Sources/_OpenAPIGeneratorCore/Translator/FileTranslator.swift
@@ -58,4 +58,14 @@ struct TranslatorContext {
     /// - Parameter string: The string to convert to be safe for Swift.
     /// - Returns: A Swift-safe version of the input string.
     var asSwiftSafeName: (String) -> String
+
+    var swiftSafeName: (String) -> (String, requiredChanges: Bool)
+
+    init(asSwiftSafeName: @escaping (String) -> (String, requiredChanges: Bool)) {
+        swiftSafeName = asSwiftSafeName
+        self.asSwiftSafeName = {
+            let (result, _) = asSwiftSafeName($0)
+            return result
+        }
+    }
 }

It would then allow the changes in my PR to use the bool for the varied logic

         /// - Parameter name: The original name.
         /// - Returns: A declaration of an enum case.
         private func translateVariableCase(_ name: String) -> Declaration {
-            let caseName = context.asSwiftSafeName(name)
-            if caseName == name {
-                return .enumCase(name: caseName, kind: .nameOnly)
-            } else {
-                return .enumCase(name: caseName, kind: .nameWithRawValue(.string(name)))
-            }
+            let (caseName, changed) = context.swiftSafeName(name)
+            return .enumCase(name: caseName, kind: changed ? .nameWithRawValue(.string(name)) : .nameOnly)
         }

What do you think?

[czechboy0]

Sure, I think that's a reasonable change, as long as there's still the existing variant that's used in most places in the codebase.

This might also be a good time to rename the closure if we want to, I know @simonjbeaumont had thoughts on the name - ideas? We need two names now, one that returns a string, and another that returns a tuple of a string and a bool.

[theoriginalbit]

I'd be interested to hear Si's thoughts on the naming.

swiftSafeName was okay, I think asSwiftSafeName is good too but I can understand trying to avoid the trap of naming things prefixed with "as". sanitizedIdentifier is my immediate thought, for the variant that provides the tuple my initial instinct would be to append IfNecessary or IfRequired, but I'm unsure if that actually communicates intent, since the implementation always rebuilds the string.

[simonjbeaumont]

What I don't love is that we have a property that stores a closure but sounds like a computed property itself: swiftSafeName sounds like it should return the safe name, not a closure that will give me a safe name, that I then have to call. sanitizedIdentifier is the same in that regard.

To keep with your suggested nomenclature but adjust to communicate use, we could use identifierSanitizer, which sounds like a thing that does a thing. Or making it a verb, like sanitizeIdentifier.

To the point about preserving the ability to call it and automatically discard the boolean, what's the motivation? Presumably to avoid having to always do sanitizeIdentifier(foo).sanitizedName when we don't care about whether it needed changes?

It would then allow the changes in my PR to use the bool for the varied logic

         /// - Parameter name: The original name.
         /// - Returns: A declaration of an enum case.
         private func translateVariableCase(_ name: String) -> Declaration {
-            let caseName = context.asSwiftSafeName(name)
-            if caseName == name {
-                return .enumCase(name: caseName, kind: .nameOnly)
-            } else {
-                return .enumCase(name: caseName, kind: .nameWithRawValue(.string(name)))
-            }
+            let (caseName, changed) = context.swiftSafeName(name)
+            return .enumCase(name: caseName, kind: changed ? .nameWithRawValue(.string(name)) : .nameOnly)
         }

I'd like to gently push back on whether we need this change at all though, because if we're just trying to tidy up code like this, it could be spelled as:

let caseName = context.swiftSafeName(name), changed = caseName != name
return .enumCase(name: caseName, kind: changed ? .nameWithRawValue(.string(name)) : .nameOnly)

...or even...

let caseName = context.swiftSafeName(name)
return .enumCase(name: caseName, kind: caseName == name ? .nameOnly : .nameWithRawValue(.string(name)))

And, that would allow the places where we don't care to not pay the cost of the additional comparison, since in the proposed change, we'd implement the one that doesn't care in terms of the one that does.

[czechboy0]

Yeah there's only one place in the generator that cares about whether they're the same, but hundreds that don't.

Regarding the naming, we've tried to avoid terms like "sanity" and instead use similar terms like "soundness", "safety", etc.

[theoriginalbit]

It was less about the spelling and more about avoiding the comparison when the result of the comparison is already known. But I do acknowledge that the change really is only for the benefit of my PR at the moment, which is why I opted to ask in an isolated question and not just make the change. I do wonder if there are other contexts that do care too and have just gone about it with comparisons.

Regarding the naming, we've tried to avoid terms like "sanity" and instead use similar terms like "soundness", "safety", etc.

Sanitize/Sanitizer would be fine though, it's not from "Sanity" but to do with cleaning.

[simonjbeaumont]

Regarding the naming, we've tried to avoid terms like "sanity" and instead use similar terms like "soundness", "safety", etc.

@theoriginalbit is correct here. Sanitization is to do with cleaning, not making something sane or implying it was previously insane.

[czechboy0]

TIL! 🙂

[theoriginalbit]

Well, I'm happy to close this if the consensus is that it won't provide any benefit.