-
Notifications
You must be signed in to change notification settings - Fork 98
Fix public extensions exposing nested code of all access levels #195
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM, just a single nitpick 🙂
Co-authored-by: Max Desiatov <max@desiatov.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your help with this, @Tunous! I was curious to see whether these changes accommodated the possibility of multiple access modifiers, such as in the case of properties with private(set)
. With b7ee28f, I added some failing test cases in testComputedPropertiesWithMultipleAccessModifiersInPublicExtension
.
For example, I believe all of the following declarations should be included in the documented interface, because they have public getters (then again, I could be wrong — Swift has some seriously complex rules around ACL):
public extension Int {
internal(set) var a: Int {
get { 1 }
set {}
}
private(set) var b: Int {
get { 1 }
set {}
}
public internal(set) var c: Int {
get { 1 }
set {}
}
public private(set) var d: Int {
get { 1 }
set {}
}
}
That is an interesting finding. I completely forgot that you could modify access level of properties like this. Having these cases included in documentation sounds like a logical thing to do. I'll try to investigate this topic a little bit further and include necessary changes later. |
@mattt I've pushed a fix and a missing test case. Looking at the grammar for access modifiers it looks like now we are covering all the possible situations. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks great. Thanks for your help with this, @Tunous!
Eventually, we'll want to codify the access-level grammar into an enumeration, rather than doing string comparisons (in fact, this is being investigated in #91).
But for now, this certainly does the job. And with the new unit tests, we can go into any future refactoring confident that things will continue to work as they should.
This pull request fixes issue where extensions marked as public would result in all nested members (no matter their access level) being added to the generated documentation.
I've included 2 tests with my fix, one based on linked issue and one with all simple cases that I've found. I'm not sure whether I've placed them in a correct location though.
Closes #194