-
Notifications
You must be signed in to change notification settings - Fork 99
Dependencies: validate Swift imports against MODULE_DEPENDENCIES and provide fix-its (rdar://150314567) #620
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
@swift-ci test |
} | ||
|
||
public init(entry: String) throws { | ||
let components = entry.split(separator: " ") |
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.
I suggest using Swift regex for this. Makes it much easier to read and understand the code flow (and no force unwraps that risk crashing if this becomes more complex in future).
guard let match = try #/(?<accessLevel>.+)? (?<moduleName>)/#.wholeMatch(in: entry)?.output else {
throw StubError.error("expected 1 or 2 space-separated components in: \(entry)")
}
self.accessLevel = try match.accessLevel.map { accessLevel in
if let a = AccessLevel(rawValue: String(accessLevel)) {
return a
} else {
throw StubError.error("unexpected access modifier '\(accessLevel)', expected one of: \(AccessLevel.allCases.map { $0.rawValue }.joined(separator: ", "))")
}
} ?? .Private
self.name = String(match.moduleName)
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.
I would like a better way to define this syntax instead of "manually" splitting like this, but I'm not sure about Swift Regex here because of https://forums.swift.org/t/should-regex-be-sendable/69529 (you can't construct the regex literal once and reuse it if it isn't Sendable). I think my preference is to keep it simple until it gets more complicated and then re-evaluate. WDYT?
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.
Fair enough but then I'd at least consider doing this:
let components = entry.split(separator: " ")
switch components.count {
case 1:
accessLevel = .Private
self.name = components[0]
case 2:
accessLevel = try AccessLevel(string: components[0])
self.name = components[1]
default:
throw StubError.error("expected 1 or 2 space-separated components in: \(entry)")
}
(elsewhere)
fileprivate extension AccessLevel {
init(string: String) throws {
guard let accessLevel = AccessLevel(rawValue: string) else {
throw StubError.error("unexpected access modifier '\(string)', expected one of: \(AccessLevel.allCases.map { $0.rawValue }.joined(separator: ", "))")
}
self = accessLevel
}
}
The main point being to localize checking in a way that makes it as obvious as possible at a glance that it's correct (and avoiding a force unwrap).
If there's a build setting assignment in xcconfig, you get a diagnostic with a fixit, otherwise if it's set on a project model entity directly (project/target), you get a diagnostic only -- right? |
c981e85
to
c6baf3b
Compare
Yup |
c6baf3b
to
05922f0
Compare
@swift-ci test |
05922f0
to
a73947b
Compare
@swift-ci test |
a353218
to
052f2b6
Compare
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.
I'm no expert in many aspects of this, but for the parts that I understand, it looks pretty good.
Looks like there's still some test failures on Linux. |
052f2b6
to
f39bce0
Compare
I changed the test to use |
…provide fix-its (rdar://150314567)
f39bce0
to
ecd4fdb
Compare
@swift-ci test |
Checks are green! @jakepetroules, any remaining concerns from you? |
@@ -40,8 +40,8 @@ struct LaunchXcode: CommandPlugin { | |||
|
|||
print("Launching Xcode...") | |||
let process = Process() | |||
process.executableURL = URL(fileURLWithPath: "/usr/bin/open") | |||
process.arguments = ["-n", "-F", "-W", "--env", "XCBBUILDSERVICE_PATH=\(buildServiceURL.path())", "-b", "com.apple.dt.Xcode"] | |||
process.executableURL = URL(fileURLWithPath: "/bin/sh") |
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.
Not sure about this one.xcode-select -p
does not necessarily return an Xcode instance, it could be Developer Command Line Tools, in which case this will just try to open /Developer and fail.
Could you run xcode-select -p
in a separate Process()
invocation, collect its output, and call open
with that path if the return value ends in .app
, otherwise fall back to -b com.apple.dt.Xcode
?
Also suggest splitting into another PR so that concerns with that don't block your primary changes.
} | ||
|
||
public init(entry: String) throws { | ||
let components = entry.split(separator: " ") |
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.
Fair enough but then I'd at least consider doing this:
let components = entry.split(separator: " ")
switch components.count {
case 1:
accessLevel = .Private
self.name = components[0]
case 2:
accessLevel = try AccessLevel(string: components[0])
self.name = components[1]
default:
throw StubError.error("expected 1 or 2 space-separated components in: \(entry)")
}
(elsewhere)
fileprivate extension AccessLevel {
init(string: String) throws {
guard let accessLevel = AccessLevel(rawValue: string) else {
throw StubError.error("unexpected access modifier '\(string)', expected one of: \(AccessLevel.allCases.map { $0.rawValue }.joined(separator: ", "))")
}
self = accessLevel
}
}
The main point being to localize checking in a way that makes it as obvious as possible at a glance that it's correct (and avoiding a force unwrap).
Dependencies: validate Swift imports against MODULE_DEPENDENCIES and provide fix-its (rdar://150314567)
Also includes fixes for: