Replace spaces with dashes in links to articles (#773)

* Replace spaces and punctuation with dashes in links to articles

rdar://119577248

* Add test that file name punctuation warn about reference collisions

* Update what punctuation is used in file names in new test

* Use simpler punctuation in file name in test for older Swift versions

* Only replace whitespaces with dashes (not punctuation)

* Move linkName function to only file where it has a reason to be used

Author: d-ronnqvist

Sources/SwiftDocC/Infrastructure/Link Resolution/PathHierarchyBasedLinkResolver.swift

@@ -106,7 +106,7 @@ final class PathHierarchyBasedLinkResolver {
     }
 
     private func addTutorial(reference: ResolvedTopicReference, source: URL, landmarks: [Landmark]) {
-        let tutorialID = pathHierarchy.addTutorial(name: urlReadablePath(source.deletingPathExtension().lastPathComponent))
+        let tutorialID = pathHierarchy.addTutorial(name: linkName(filename: source.deletingPathExtension().lastPathComponent))
         resolvedReferenceMap[tutorialID] = reference
 
         for landmark in landmarks {
@@ -119,7 +119,7 @@ final class PathHierarchyBasedLinkResolver {
     func addTechnology(_ technology: DocumentationContext.SemanticResult<Technology>) {
         let reference = technology.topicGraphNode.reference
 
-        let technologyID = pathHierarchy.addTutorialOverview(name: urlReadablePath(technology.source.deletingPathExtension().lastPathComponent))
+        let technologyID = pathHierarchy.addTutorialOverview(name: linkName(filename: technology.source.deletingPathExtension().lastPathComponent))
         resolvedReferenceMap[technologyID] = reference
 
         var anonymousVolumeID: ResolvedIdentifier?
@@ -149,21 +149,20 @@ final class PathHierarchyBasedLinkResolver {
 
     /// Adds a technology root article and its headings to the path hierarchy.
     func addRootArticle(_ article: DocumentationContext.SemanticResult<Article>, anchorSections: [AnchorSection]) {
-        let articleID = pathHierarchy.addTechnologyRoot(name: article.source.deletingPathExtension().lastPathComponent)
+        let linkName = linkName(filename: article.source.deletingPathExtension().lastPathComponent)
+        let articleID = pathHierarchy.addTechnologyRoot(name: linkName)
         resolvedReferenceMap[articleID] = article.topicGraphNode.reference
         addAnchors(anchorSections, to: articleID)
     }
 
     /// Adds an article and its headings to the path hierarchy.
     func addArticle(_ article: DocumentationContext.SemanticResult<Article>, anchorSections: [AnchorSection]) {
-        let articleID = pathHierarchy.addArticle(name: article.source.deletingPathExtension().lastPathComponent)
-        resolvedReferenceMap[articleID] = article.topicGraphNode.reference
-        addAnchors(anchorSections, to: articleID)
+        addArticle(filename: article.source.deletingPathExtension().lastPathComponent, reference: article.topicGraphNode.reference, anchorSections: anchorSections)
     }
 
     /// Adds an article and its headings to the path hierarchy.
     func addArticle(filename: String, reference: ResolvedTopicReference, anchorSections: [AnchorSection]) {
-        let articleID = pathHierarchy.addArticle(name: filename)
+        let articleID = pathHierarchy.addArticle(name: linkName(filename: filename))
         resolvedReferenceMap[articleID] = reference
         addAnchors(anchorSections, to: articleID)
     }
@@ -186,7 +185,7 @@ final class PathHierarchyBasedLinkResolver {
     /// Adds a task group on a given page to the documentation hierarchy.
     func addTaskGroup(named name: String, reference: ResolvedTopicReference, to parent: ResolvedTopicReference) {
         let parentID = resolvedReferenceMap[parent]!
-        let taskGroupID = pathHierarchy.addNonSymbolChild(parent: parentID, name: urlReadablePath(name), kind: "taskGroup")
+        let taskGroupID = pathHierarchy.addNonSymbolChild(parent: parentID, name: urlReadableFragment(name), kind: "taskGroup")
         resolvedReferenceMap[taskGroupID] = reference
     }
 
@@ -272,3 +271,19 @@ final class PathHierarchyBasedLinkResolver {
         return result
     }
 }
+
+/// Creates a more writable version of an articles file name for use in documentation links.
+///
+/// Compared to `urlReadablePath(_:)` this preserves letters in other written languages.
+private func linkName<S: StringProtocol>(filename: S) -> String {
+    // It would be a nice enhancement to also remove punctuation from the filename to allow an article in a file named "One, two, & three!"
+    // to be referenced with a link as `"One-two-three"` instead of `"One,-two-&-three!"` (rdar://120722917)
+    return filename
+        // Replace continuous whitespace and dashes
+        .components(separatedBy: whitespaceAndDashes)
+        .filter({ !$0.isEmpty })
+        .joined(separator: "-")
+}
+
+private let whitespaceAndDashes = CharacterSet.whitespaces
+    .union(CharacterSet(charactersIn: "-–—")) // hyphen, en dash, em dash

Sources/SwiftDocC/Model/Identifier.swift

@@ -643,6 +643,7 @@ func urlReadablePath<S: StringProtocol>(_ path: S) -> String {
 }
 
 private extension CharacterSet {
+    // For fragments
     static let fragmentCharactersToRemove = CharacterSet.punctuationCharacters // Remove punctuation from fragments
         .union(CharacterSet(charactersIn: "`"))       // Also consider back-ticks as punctuation. They are used as quotes around symbols or other code.
         .subtracting(CharacterSet(charactersIn: "-")) // Don't remove hyphens. They are used as a whitespace replacement.
@@ -669,3 +670,4 @@ func urlReadableFragment<S: StringProtocol>(_ fragment: S) -> String {
 
     return fragment
 }
+

Tests/SwiftDocCTests/Infrastructure/DocumentationContext/DocumentationContextTests.swift

@@ -1736,6 +1736,53 @@ let expected = """
         XCTAssertEqual("/=(_:_:)",  pageIdentifiersAndNames["/documentation/Operators/MyNumber/_=(_:_:)-3m4ko"])
     }
 
+    func testFileNamesWithDifferentPunctuation() throws {
+        let tempURL = try createTempFolder(content: [
+            Folder(name: "unit-test.docc", content: [
+                TextFile(name: "Hello-world.md", utf8Content: """
+                # Dash
+                
+                No whitespace in the file name
+                """),
+                
+                TextFile(name: "Hello world.md", utf8Content: """
+                # Only space
+                
+                This has the same reference as "Hello-world.md" and will raise a warning.
+                """),
+                
+                TextFile(name: "Hello  world.md", utf8Content: """
+                # Multiple spaces
+                
+                Each space is replaced with a dash in the reference, so this has a unique reference.
+                """),
+                
+                TextFile(name: "Hello, world!.md", utf8Content: """
+                # Space and punctuation
+                
+                The punctuation is not removed from the reference, so this has a unique reference.
+                """),
+                
+                TextFile(name: "Hello. world?.md", utf8Content: """
+                # Space and different punctuation
+                
+                The punctuation is not removed from the reference, so this has a unique reference.
+                """),
+            ])
+        ])
+        let (_, _, context) = try loadBundle(from: tempURL)
+
+        XCTAssertEqual(context.problems.map(\.diagnostic.summary), ["Redeclaration of 'Hello world.md'; this file will be skipped"])
+        
+        XCTAssertEqual(context.knownPages.map(\.absoluteString).sorted(), [
+            "doc://unit-test/documentation/unit-test",
+            "doc://unit-test/documentation/unit-test/Hello,-world!",
+            "doc://unit-test/documentation/unit-test/Hello--world",
+            "doc://unit-test/documentation/unit-test/Hello-world",
+            "doc://unit-test/documentation/unit-test/Hello.-world-",
+        ])
+    }
+    
     func testSpecialCharactersInLinks() throws {
         let originalSymbolGraph = Bundle.module.url(forResource: "TestBundle", withExtension: "docc", subdirectory: "Test Bundles")!.appendingPathComponent("mykit-iOS.symbols.json")
 
@@ -1751,7 +1798,15 @@ let expected = """
             """),
 
             TextFile(name: "article-with-😃-in-filename.md", utf8Content: """
-            # Article with 😃 emoji in file name
+            # Article with 😃 emoji in its filename
+            
+            Abstract
+            
+            ### Hello world
+            """),
+            
+            TextFile(name: "Article: with - various! whitespace & punctuation. in, filename.md", utf8Content: """
+            # Article with various whitespace and punctuation in its filename
             
             Abstract
             
@@ -1767,6 +1822,8 @@ let expected = """
             - <doc:article-with-emoji-in-heading#Hello-🌍>
             - <doc:article-with-😃-in-filename>
             - <doc:article-with-😃-in-filename#Hello-world>
+            - <doc:Article:-with-various!-whitespace-&-punctuation.-in,-filename>
+            - <doc:Article:-with-various!-whitespace-&-punctuation.-in,-filename#Hello-world>
             
             Now test the same links in topic curation.
             
@@ -1776,6 +1833,7 @@ let expected = """
             
             - ``MyClass/myFunc🙂()``
             - <doc:article-with-😃-in-filename>
+            - <doc:Article:-with-various!-whitespace-&-punctuation.-in,-filename>
             """),
         ])
         let bundleURL = try testBundle.write(inside: createTemporaryDirectory())
@@ -1789,11 +1847,12 @@ let expected = """
 
         let moduleSymbol = try XCTUnwrap(entity.semantic as? Symbol)
         let topicSection = try XCTUnwrap(moduleSymbol.topics?.taskGroups.first)
-        
+
         // Verify that all the links in the topic section resolved
         XCTAssertEqual(topicSection.links.map(\.destination), [
             "doc://special-characters/documentation/MyKit/MyClass/myFunc_()",
             "doc://special-characters/documentation/special-characters/article-with---in-filename",
+            "doc://special-characters/documentation/special-characters/Article:-with---various!-whitespace-&-punctuation.-in,-filename",
         ])
 
         // Verify that all resolved link exist in the context.
@@ -1808,10 +1867,11 @@ let expected = """
         let renderNode = translator.visit(moduleSymbol) as! RenderNode
 
         // Verify that the resolved links rendered as links
-        XCTAssertEqual(renderNode.topicSections.first?.identifiers.count, 2)
+        XCTAssertEqual(renderNode.topicSections.first?.identifiers.count, 3)
         XCTAssertEqual(renderNode.topicSections.first?.identifiers, [
             "doc://special-characters/documentation/MyKit/MyClass/myFunc_()",
             "doc://special-characters/documentation/special-characters/article-with---in-filename",
+            "doc://special-characters/documentation/special-characters/Article:-with---various!-whitespace-&-punctuation.-in,-filename",
         ])
 
 
@@ -1826,7 +1886,7 @@ let expected = """
 
         XCTAssertEqual(lists.count, 1)
         let list = try XCTUnwrap(lists.first)
-        XCTAssertEqual(list.items.count, 4, "Unexpected list items: \(list.items.map(\.content))")
+        XCTAssertEqual(list.items.count, 6, "Unexpected list items: \(list.items.map(\.content))")
 
         func withContentAsReference(_ listItem: RenderBlockContent.ListItem?, verify: (RenderReferenceIdentifier, Bool, String?, [RenderInlineContent]?) -> Void) {
             guard let listItem = listItem else {
@@ -1866,7 +1926,19 @@ let expected = """
             XCTAssertEqual(overridingTitle, nil)
             XCTAssertEqual(overridingTitleInlineContent, nil)
         }
-        
+        withContentAsReference(list.items.dropFirst(4).first) { identifier, isActive, overridingTitle, overridingTitleInlineContent in
+            XCTAssertEqual(identifier.identifier, "doc://special-characters/documentation/special-characters/Article:-with---various!-whitespace-&-punctuation.-in,-filename")
+            XCTAssertEqual(isActive, true)
+            XCTAssertEqual(overridingTitle, nil)
+            XCTAssertEqual(overridingTitleInlineContent, nil)
+        }
+        withContentAsReference(list.items.dropFirst(5).first) { identifier, isActive, overridingTitle, overridingTitleInlineContent in
+            XCTAssertEqual(identifier.identifier, "doc://special-characters/documentation/special-characters/Article:-with---various!-whitespace-&-punctuation.-in,-filename#Hello-world")
+            XCTAssertEqual(isActive, true)
+            XCTAssertEqual(overridingTitle, nil)
+            XCTAssertEqual(overridingTitleInlineContent, nil)
+        }
+    
         // Verify that the topic render references have titles with special characters when the original content contained special characters
         XCTAssertEqual(
             (renderNode.references["doc://special-characters/documentation/MyKit/MyClass/myFunc_()"] as? TopicRenderReference)?.title,
@@ -1878,12 +1950,20 @@ let expected = """
         )
         XCTAssertEqual(
             (renderNode.references["doc://special-characters/documentation/special-characters/article-with---in-filename"] as? TopicRenderReference)?.title,
-            "Article with 😃 emoji in file name"
+            "Article with 😃 emoji in its filename"
         )
         XCTAssertEqual(
             (renderNode.references["doc://special-characters/documentation/special-characters/article-with---in-filename#Hello-world"] as? TopicRenderReference)?.title,
             "Hello world"
         )
+        XCTAssertEqual(
+            (renderNode.references["doc://special-characters/documentation/special-characters/Article:-with---various!-whitespace-&-punctuation.-in,-filename"] as? TopicRenderReference)?.title,
+            "Article with various whitespace and punctuation in its filename"
+        )
+        XCTAssertEqual(
+            (renderNode.references["doc://special-characters/documentation/special-characters/Article:-with---various!-whitespace-&-punctuation.-in,-filename#Hello-world"] as? TopicRenderReference)?.title,
+            "Hello world"
+        )
     }
 
     func testNonOverloadCollisionFromExtension() throws {