Option for specifying which access levels are included #91
Conversation
| public enum AccessLevel: String, Codable { | ||
| case `public` | ||
| case `internal` | ||
| case `private` |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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...)
There was a problem hiding this comment.
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.
| @@ -58,7 +58,26 @@ public final class Symbol { | |||
|
|
|||
| return false | |||
| } | |||
|
|
|||
|
|
|||
| public var isInternal: Bool { | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
.@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).
| @@ -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 | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
(@johankool Don't worry about trying to make sense of my rambling commentary here. I'll provide more actionable feedback in my review)
There was a problem hiding this comment.
Agreed. It didn’t feel quite right to put it here. Not quite sure what the right place would be though.
| @@ -55,3 +55,9 @@ extension Subscript: API { | |||
|
|
|||
| extension Typealias: API {} | |||
| extension Variable: API {} | |||
|
|
|||
| public enum AccessLevel: String, Codable { | |||
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
Thanks for taking this on, @johankool. I'm taking a look at this now. |
|
Hi, this looks interesting, is there any detail/requirement missing to move forward with this work? |
|
Hello! any update on this feature? |
|
@muizidn Sorry, no, I haven't looked at this anymore. |
|
I think this is now properly fixed in #219. |
Implements #72
Not yet tested very extensively.