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.

Sign up for GitHub

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

Jump to bottom

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

Closed
wants to merge 6 commits into from
Closed

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

wants to merge 6 commits into from

Conversation

iainsmith
Copy link

👋 @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.
  • Ensure standardOutput, standardInput & standardError are not closed.
  • 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

gwikiera and others added 5 commits April 11, 2018 23:41
@gwikiera @iainsmith
Added Handle protocol to get asynchronous output
bfa2cdd
@iainsmith
Fixed bug with xcodebuild test
bfc5ebb
@iainsmith
Keep standardOuput, standardError & standardInput fileHandles open.
295a25c 
This adds two tests with no assertions to verify that standardOut & standardError are not closed.
I tried wrapping the shellOut calls in XCTAssertNoThrow but this causes the tests to always pass.

Co-authored-by: SteveBarnegren <steve.barnegren@candyspace.com>
@iainsmith
Run CI on Linux & OSX against Swift 4.0 & 4.1
1091c9c
@iainsmith
Knock swift version down to 4.0
46c6382
JohnSundell
JohnSundell reviewed Apr 12, 2018
View reviewed changes
Copy link
Owner

@JohnSundell JohnSundell left a comment •
edited

Choose a reason for hiding this comment

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

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
Copy link
Author

iainsmith commented Apr 13, 2018 •
edited

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 iainsmith mentioned this pull request Apr 13, 2018
Closed
@iainsmith iainsmith changed the title Async Output & Fixed standard FileHandler's Fix Filehandler bugs & Swift 4 package comment. Groundwork for Async output Apr 13, 2018
@iainsmith
Copy link
Author

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

@f-meloni f-meloni mentioned this pull request Dec 5, 2018
Closed
@daneov
Copy link

daneov commented Feb 6, 2019

Anything that needs to be done for this? :)

@daneov daneov mentioned this pull request Feb 7, 2019
Merged
@iainsmith
Copy link
Author

Closing due to lack of feedback.

@iainsmith iainsmith closed this Feb 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JohnSundell JohnSundell JohnSundell left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Incomplete standard output Async output?
4 participants
@iainsmith @daneov @JohnSundell @gwikiera