Skip to content
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

Use Swift.Result instead of our Result if compiler(>=5) #280

Merged
merged 2 commits into from
Jan 10, 2019

Conversation

norio-nomura
Copy link
Contributor

No description provided.

@norio-nomura
Copy link
Contributor Author

This makes it easier for users using antitypical/Result to switch to Swift.Result.

I confirmed that Commandant can be built with this and using Swift.Result:
final func run(command verb: Swift.String, arguments: [Swift.String]) -> Swift.Result<(), Commandant.CommandantError<ClientError>>?
as following:

$ docker run --privileged -it -v `pwd`:`pwd` -w `pwd` --rm norionomura/swift:swift-5.0-branch swift run --repl -Xswiftc -swift-version -Xswiftc 5
[1/1] Linking ./.build/x86_64-unknown-linux/debug/libCommandant__REPL.so
Launching Swift REPL with arguments: -I/Users/norio/github/swift-dev/SourceKitten/Carthage/Checkouts/Commandant/.build/x86_64-unknown-linux/debug -L/Users/norio/github/swift-dev/SourceKitten/Carthage/Checkouts/Commandant/.build/x86_64-unknown-linux/debug -lCommandant__REPL
Welcome to Swift version 5.0-dev (LLVM eb266bb301, Clang 2b49a6d7dc, Swift 5bf394d8ff).
Type :help for assistance.
  1> import Commandant
  2> :type lookup CommandRegistry
final class CommandRegistry<ClientError> where ClientError : Error {
  @_hasInitialValue final var commandsByVerb: [Swift.String : Commandant.CommandWrapper<ClientError>]
  final var commands: [Commandant.CommandWrapper<ClientError>] {
    get
  }
  init()
  @discardableResult final func register<C>(_ commands: C...) -> Commandant.CommandRegistry<ClientError> where ClientError == C.ClientError, C : CommandProtocol
  final func run(command verb: Swift.String, arguments: [Swift.String]) -> Swift.Result<(), Commandant.CommandantError<ClientError>>?
  final subscript(verb: Swift.String) -> Commandant.CommandWrapper<ClientError>? {
    get
  }
  deinit
}
extension CommandRegistry {
  final func main(defaultVerb: Swift.String, errorHandler: (ClientError) -> ()) -> Swift.Never
  final func main(arguments: [Swift.String], defaultVerb: Swift.String, errorHandler: (ClientError) -> ()) -> Swift.Never
  final func executeSubcommandIfExists(_ executableName: Swift.String, verb: Swift.String, arguments: [Swift.String]) -> Swift.Int32?
}

  2>  

Copy link
Contributor

@NachoSoto NachoSoto left a comment

Choose a reason for hiding this comment

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

Nice!

@@ -1,10 +1,35 @@
// Copyright (c) 2015 Rob Rix. All rights reserved.

#if swift(>=4.2)
#if compiler(>=5)
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think this maintains the requirements for semantic version compatibility, so I think we need to drop support for Swift 4 to do this. (And we should do this!)

If we conditionally define the enum, then the output is different if Result is compiled in Swift 4 mode or Swift 5 mode. Since semantic versioning was defined by TPW and he writes Ruby, it's not totally clear whether it's meant to be source- or binary-compatible, but I think the safe thing is to require both.

Copy link
Contributor Author

@norio-nomura norio-nomura Jan 10, 2019

Choose a reason for hiding this comment

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

I think we need to drop support for Swift 4 to do this. (And we should do this!)

Is it enough to just change the major (or minor) version? It seems there is no need to make Build impossible with older Swift.🤔

If we conditionally define the enum, then the output is different if Result is compiled in Swift 4 mode or Swift 5 mode.

It is not affected by the -swift-version flag. When using the Swift 5+ compiler, Swift.Result will be used.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, edited previous comment. 🙇

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I see! Tricky!

@mdiep
Copy link
Contributor

mdiep commented Jan 10, 2019

I'm very excited to see this! 👏🏻

@norio-nomura norio-nomura merged commit c083834 into master Jan 10, 2019
@norio-nomura norio-nomura deleted the use-stdlib-result branch January 10, 2019 13:54
@norio-nomura
Copy link
Contributor Author

Thanks! 🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants