Add 'open' to all public classes and their members. #150
Conversation
This can probably be refined by the XCTest team in a later commit. In preparation for landing SE-0117.
| @@ -39,17 +39,17 @@ public class XCTest { | |||
| /// ensure compatibility of tests between swift-corelibs-xctest and Apple | |||
| /// XCTest, you should not set this property. See | |||
| /// https://bugs.swift.org/browse/SR-1129 for details. | |||
| public public(set) var testRun: XCTestRun? = nil | |||
| open open(set) var testRun: XCTestRun? = nil | |||
There was a problem hiding this comment.
@rjmccall, is this correct?
|
Looks good to me, thanks @jrose-apple! A later commit should probably revise some of these.
Thoughts, @mike-ferris-apple, @briancroom? |
|
I’d say that first goal should be equivalence with Xcode's XCTest… That said, it might be interesting to assess the API and make changes to the swift side of both Xcode’s and CoreLib's XCTest in concert. Mike
|
|
@mike-ferris-apple I believe swiftlang/swift#3882 will import all Objective-C classes from Xcode XCTest as By my comment above, I meant I think both the Swift overlay for Xcode XCTest and swift-corelibs-xctest should declare |
I am not up to speed on exactly what we’re solving by introducing this distinction, also, and I would be somewhat cautious about cutting off subclassability just because we can’t think of a reason you’d want to… For example, does closing off XCTestSuite and XCTestSuiteRun gain us anything? It certainly loses us the case of someone discovering something cool they could do by doing this only to find that they can’t. I need a philosophy lesson (and perhaps technology lesson) as to the appropriate use for this distinction in Swift. I’d say that at the very least this requires some fairly deep thinking. And probably that thinking should consider how prepared the rest of the overall context in which XCTest exists (on the Xcode side) is affected. in fact, what the IDE and larger testing infrastructure is designed/prepared for is potentially one of the more compelling reasons to close stuff off... Mike |
|
Awesome, thanks for the detailed response. I'm looking forward to hearing what the XCTest team decides on which APIs should be Anyway, sorry for the noise on this pull request... 😅 |
|
Okay, we're just going to merge this in; thanks, all. |
This can probably be refined by the XCTest team in a later commit.
In preparation for landing SE-0117.