Skip to content
This repository was archived by the owner on Jun 1, 2023. It is now read-only.

Option for specifying which access levels are included #91

Closed
wants to merge 2 commits into from
Closed

Option for specifying which access levels are included #91

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ac0ad38
Option for specifying which access levels are included
johankool Apr 20, 2020
93c6eab
Merge branch 'master' into access-level-option
johankool Apr 20, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions Sources/SwiftDoc/API.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,3 +55,9 @@ extension Subscript: API {

extension Typealias: API {}
extension Variable: API {}

public enum AccessLevel: String, Codable {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One fancy thought: Instead of using String as the raw representable, we could also use Int and make the enum conform to Comparable. Then we could do nice expressions like if minimumAccessLevel >= accessLevel.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

BTW, if it goes that route and supports Comparable, should have a note added that it won't be needed with Swift 5.3 thanks to SE-0266.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've already started work with the latest 5.3 snapshot to get package resources working, so I'll take a look at using automatic Comparable synthesis with an explicit implementation for < in a conditional compilation block.

case `public`
case `internal`
case `private`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it makes sense to also the fileprivate access level as this enum also contains private, which is a lower access level than fileprivate.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Personally, I see fileprivate as being equivalent to private for the purposes of swift-doc, in the same way that public implies open (though admittedly, I don't fully understand the use case for documenting private declarations, so maybe I'm missing a key distinction...)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess making distinction between private and. fileprivate is indeed unneeded for this. It think normally I’d want the internal methods of my app documented and wouldn’t care so much about the private methods. But perhaps some apps have sufficient complexity that also documenting what private methods do is useful.

}
8 changes: 5 additions & 3 deletions Sources/SwiftDoc/Interface.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,12 @@ import struct SwiftSemantics.Protocol
public final class Interface: Codable {
public let imports: [Import]
public let symbols: [Symbol]
public let minimumAccessLevel: AccessLevel
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In my point of view, adding this property on the interface mixes concerns. The setting for creating the documentation should not be on a structure that describes source code.

I'd suggest we don't include it as a property, but instead only pass the symbols that have the correct ACL. Same goes for the extension of Report. Alternatively we could collect and work with all symbols and then filter when we generate the documentation. But I did not fully think this through, so there might be some implications I did not consider.

But I also think @mattt is the person to decide on this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @Lukas-Stuehrk here. An Interface should represent a fixed collection of symbols and relationships. For instance, if we were to generate the public API for a library on macOS vs. Linux, we'd associate a separate Interface value for each platform.

As to where that determination should be made, that remains an open question. Like Lukas said, it could be done either in "model" layer (by adding a minimumAccessLevel parameter to init(sourceFiles:)) or in the "view" layer (by having each page / component filter symbols according to a minimumAccessLevel property). The former would be simpler, but I'm not 100% sure whether we can correctly reconcile relationships without knowledge of every symbol. For example, I know that a public class can't inherit from a private class, but I haven't thought through all of the possible interactions between protocols, types, and members.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(@johankool Don't worry about trying to make sense of my rambling commentary here. I'll provide more actionable feedback in my review)

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. It didn’t feel quite right to put it here. Not quite sure what the right place would be though.


public required init(imports: [Import], symbols: [Symbol]) {
public required init(imports: [Import], symbols: [Symbol], minimumAccessLevel: AccessLevel) {
self.imports = imports
self.symbols = symbols.filter { $0.isPublic }
self.symbols = symbols.filter { $0.isIncluded(minimumAccessLevel: minimumAccessLevel) }
self.minimumAccessLevel = minimumAccessLevel
}

// MARK: -
Expand Down Expand Up @@ -37,7 +39,7 @@ public final class Interface: Codable {
var superclasses = Set(CollectionOfOne(baseClass))

while !superclasses.isEmpty {
let subclasses = Set(superclasses.flatMap { typesInheriting(from: $0) }.filter { $0.isPublic })
let subclasses = Set(superclasses.flatMap { typesInheriting(from: $0) }.filter { $0.isIncluded(minimumAccessLevel: self.minimumAccessLevel) })
defer { superclasses = subclasses }
classClusters[baseClass, default: []].formUnion(subclasses)
}
Expand Down
11 changes: 7 additions & 4 deletions Sources/SwiftDoc/Module.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,18 +7,21 @@ public final class Module: Codable {

public let sourceFiles: [SourceFile]

public let minimumAccessLevel: AccessLevel

public lazy var interface: Interface = {
let imports = sourceFiles.flatMap { $0.imports }
let symbols = sourceFiles.flatMap { $0.symbols }
return Interface(imports: imports, symbols: symbols)
return Interface(imports: imports, symbols: symbols, minimumAccessLevel: minimumAccessLevel)
}()

public required init(name: String = "Anonymous", sourceFiles: [SourceFile]) {
public required init(name: String = "Anonymous", sourceFiles: [SourceFile], minimumAccessLevel: AccessLevel) {
self.name = name
self.sourceFiles = sourceFiles
self.minimumAccessLevel = minimumAccessLevel
}

public convenience init(name: String = "Anonymous", paths: [String]) throws {
public convenience init(name: String = "Anonymous", paths: [String], minimumAccessLevel: AccessLevel) throws {
var sources: [(file: URL, directory: URL)] = []

let fileManager = FileManager.default
Expand All @@ -38,6 +41,6 @@ public final class Module: Codable {

let sourceFiles = try sources.parallelMap { try SourceFile(file: $0.file, relativeTo: $0.directory) }

self.init(name: name, sourceFiles: sourceFiles)
self.init(name: name, sourceFiles: sourceFiles, minimumAccessLevel: minimumAccessLevel)
}
}
21 changes: 20 additions & 1 deletion Sources/SwiftDoc/Symbol.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,26 @@ public final class Symbol {

return false
}


public var isInternal: Bool {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think either the implementation or the property name is highly misleading. With this implementation, a public property would return true. Same for an open or fileprivate property.

Also, the actual decision whether a symbol is internal is way more complicated. There are quite some cases to consider, especially with extensions. You can take a look at the existing implementation of isPublic where these cases are considered.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.@Lukas-Stuehrk is correct. Determining access levels for symbols is extremely complicated in Swift. I've had to fix the implementation for isPublic a few times already to accommodate unforeseen edge cases (e.g. cases of public enumerations are always public, even though they never have access modifiers).

if api.modifiers.contains(where: { $0.name == "private" }) {
return false
}

return true
}

public func isIncluded(minimumAccessLevel: AccessLevel) -> Bool {
switch minimumAccessLevel {
case .public:
return isPublic
case .internal:
return isInternal
case .private:
return true
}
}

public var isDocumented: Bool {
return documentation?.isEmpty == false
}
Expand Down
2 changes: 1 addition & 1 deletion Sources/swift-doc/Extensions/DCOV+Extensions.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ extension Report {
public init(module: Module) {
let entries = module.sourceFiles
.flatMap { $0.symbols }
.filter { $0.isPublic }
.filter { $0.isIncluded(minimumAccessLevel: module.minimumAccessLevel) }
.map { Entry($0) }

self.init(entries: entries)
Expand Down
7 changes: 6 additions & 1 deletion Sources/swift-doc/Subcommands/Coverage.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,11 @@ extension SwiftDoc {
@Argument(help: "One or more paths to Swift files")
var inputs: [String]

@Option(name: .customLong("minimum-access-level"),
default: .public,
help: "The minimum access level for declarations to be included")
var minimumAccessLevel: AccessLevel

@Option(name: .shortAndLong,
help: "The path for generated report")
var output: String?
Expand All @@ -20,7 +25,7 @@ extension SwiftDoc {
var options: Options

func run() throws {
let module = try Module(paths: options.inputs)
let module = try Module(paths: options.inputs, minimumAccessLevel: options.minimumAccessLevel)
let report = Report(module: module)

if let output = options.output {
Expand Down
9 changes: 7 additions & 2 deletions Sources/swift-doc/Subcommands/Diagram.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,11 @@ extension SwiftDoc {
struct Options: ParsableArguments {
@Argument(help: "One or more paths to Swift files")
var inputs: [String]

@Option(name: .customLong("minimum-access-level"),
default: .public,
help: "The minimum access level for declarations to be included")
var minimumAccessLevel: AccessLevel
}

static var configuration = CommandConfiguration(abstract: "Generates diagram of Swift symbol relationships")
Expand All @@ -19,7 +24,7 @@ extension SwiftDoc {
var options: Options

func run() throws {
let module = try Module(paths: options.inputs)
let module = try Module(paths: options.inputs, minimumAccessLevel: options.minimumAccessLevel)
print(diagram(of: module), to: &standardOutput)
}
}
Expand Down Expand Up @@ -61,7 +66,7 @@ fileprivate func diagram(of module: Module) -> String {
}


for symbol in (module.interface.symbols.filter { $0.isPublic && $0.api is Type }) {
for symbol in (module.interface.symbols.filter { $0.isIncluded(minimumAccessLevel: module.minimumAccessLevel) && $0.api is Type }) {
let symbolNode = Node("\(symbol.id)")
graph.append(symbolNode)

Expand Down
11 changes: 9 additions & 2 deletions Sources/swift-doc/Subcommands/Generate.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,8 @@ import SwiftMarkup
import SwiftSemantics
import struct SwiftSemantics.Protocol

extension AccessLevel: ExpressibleByArgument { }

extension SwiftDoc {
struct Generate: ParsableCommand {
enum Format: String, ExpressibleByArgument {
Expand All @@ -30,6 +32,11 @@ extension SwiftDoc {
help: "The output format")
var format: Format

@Option(name: .customLong("minimum-access-level"),
default: .public,
help: "The minimum access level for declarations to be included")
var minimumAccessLevel: AccessLevel

@Option(name: .customLong("base-url"),
default: "/",
help: "The base URL used for all relative URLs in generated documents.")
Expand All @@ -42,7 +49,7 @@ extension SwiftDoc {
var options: Options

func run() throws {
let module = try Module(name: options.moduleName, paths: options.inputs)
let module = try Module(name: options.moduleName, paths: options.inputs, minimumAccessLevel: options.minimumAccessLevel)

let outputDirectoryURL = URL(fileURLWithPath: options.output)
try fileManager.createDirectory(at: outputDirectoryURL, withIntermediateDirectories: true, attributes: fileAttributes)
Expand All @@ -53,7 +60,7 @@ extension SwiftDoc {
var pages: [String: Page] = [:]

var globals: [String: [Symbol]] = [:]
for symbol in module.interface.topLevelSymbols.filter({ $0.isPublic }) {
for symbol in module.interface.topLevelSymbols.filter({ $0.isIncluded(minimumAccessLevel: options.minimumAccessLevel) }) {
switch symbol.api {
case is Class, is Enumeration, is Structure, is Protocol:
pages[path(for: symbol)] = TypePage(module: module, symbol: symbol)
Expand Down
2 changes: 1 addition & 1 deletion Sources/swift-doc/Supporting Types/Pages/HomePage.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@ struct HomePage: Page {
init(module: Module) {
self.module = module

for symbol in module.interface.topLevelSymbols.filter({ $0.isPublic }) {
for symbol in module.interface.topLevelSymbols.filter({ $0.isIncluded(minimumAccessLevel: module.minimumAccessLevel) }) {
switch symbol.api {
case is Class:
classes.append(symbol)
Expand Down
2 changes: 1 addition & 1 deletion Sources/swift-doc/Supporting Types/Pages/SidebarPage.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@ struct SidebarPage: Page {
init(module: Module) {
self.module = module

for symbol in module.interface.topLevelSymbols.filter({ $0.isPublic }) {
for symbol in module.interface.topLevelSymbols.filter({ $0.isIncluded(minimumAccessLevel: module.minimumAccessLevel) }) {
switch symbol.api {
case is Class:
typeNames.insert(symbol.id.description)
Expand Down
2 changes: 1 addition & 1 deletion Tests/SwiftDocTests/NestedTypesTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ final class NestedTypesTests: XCTestCase {

let url = try temporaryFile(contents: source)
let sourceFile = try SourceFile(file: url, relativeTo: url.deletingLastPathComponent())
let module = Module(name: "Module", sourceFiles: [sourceFile])
let module = Module(name: "Module", sourceFiles: [sourceFile], minimumAccessLevel: .public)

XCTAssertEqual(sourceFile.symbols.count, 4)

Expand Down