JohnSundell / ShellOut

Easily run shell commands from a Swift script or command line tool
MIT License
870 stars 83 forks source link

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

Closed iainsmith closed 4 years ago

iainsmith commented 6 years ago

đź‘‹ @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

Partially Reverted

iainsmith commented 6 years ago

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


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. #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?”
      1. 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.
    2. Should we add and support the concreteStringHandle type?
      1. I actually think we should drop this from the PR.
        1. It’s trivial for people to build their own
        2. 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 commented 6 years ago

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

daneov commented 5 years ago

Anything that needs to be done for this? :)

iainsmith commented 4 years ago

Closing due to lack of feedback.