Fix Filehandler bugs & Swift 4 package comment. Groundwork for Async output

[iainsmith]

👋 @JohnSundell.

This PR is a combination of work from @gwikiera & @SteveBarnegren (Added as a coauthor to the appropriate commit). If we merge this PR we can close #30 & #23.

Changes

• Add Handler protocol enabling custom async output.
  • Fixes Async output? #11 (Async output)
• Ensure standardOutput, standardInput & standardError are not closed.
  • Fixes Incomplete standard output #18 (Incomplete Standard output)
• Configure Travis to run tests on MacOS & Linux against Swift 4.0 & 4.1

Partially Reverted

• Support Swift4.1 #31 - Package.swift. The tools comment should remain at 4.0 otherwise consumers can't build ShellOut with swift 4.0

[JohnSundell]

Thanks for this @iainsmith. I'm thinking if this is the right way to go API design wise 🤔 When I want async output it almost seems like a separate top level function should be used, something like:

shellOut(to: "command") { output in
    ...
}

I realize this would lead to some minor code duplication (or rather, that we have to maintain more top level functions). But I think it would lead to a much nicer to use API.

What do you think?

[iainsmith]

TL;DR. I’ve updated the PR to remove StringHandle. IMO this gives us the best of all worlds.

• Fix key bugs
• Non breaking improvements to the public API
• No additional concrete types that we’re committing to long term.
• Flexibility to discuss better streaming API’s in another issue. New Async/Streaming Output API. #33

---

So it seems like there are a few things to discuss here.

1. What’s a more ergonomic API for streaming output that we can add as new (non breaking API)?
2. How can we fix the current FileHandle bugs & improve the public API we are already committed to in the 2.x versioning cycle? How do we avoid adding unnecessary public API we don’t want to support.

I’d like to propose we can move forward by:

1. Breaking out discussion of the new streaming API into a separate issue. We can take a bit more time to get it right rather than rush it into this PR. New Async/Streaming Output API. #33
   • How do we support colorised output in a nice way.
   • Do we want output as Data or a String?
2. Is this PR a good change to the public API?
   1. I think the key question here is, “Should we add the Handle Protocol?”
   2. I’m a big fan of adding this.
      1. It’s a great way to enable custom streaming output with a non breaking public API change.
      2. It’s unlikely we will ever extend this protocol (and if we have to we can probably add a default implementation).
      3. We can also use this protocol to build the new streaming public methods that we’d like.
   3. Should we add and support the concreteStringHandle type?
      1. I actually think we should drop this from the PR.
      2. It’s trivial for people to build their own
      3. We want to encourage people to use the new streaming API once it’s available.

We can always add StringHandle in a future PR if we want to.

Here is an example of how I'm using the Handle protocol in [swift-docker] for streaming long running docker commands: (https://github.com/iainsmith/swift-docker/blob/7653086b109033cb47a63c175e90c1b7f80002f2/Sources/SwiftDockerLib/Logging.swift#L22L37)

[iainsmith]

I also realised the original title of the PR wasn't that helpful 🤦‍♂️

[daneov]

Anything that needs to be done for this? :)

[iainsmith]

Closing due to lack of feedback.