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

An asynchronous version of call to shellOut #34

Open
wants to merge 16 commits into
base: master
Choose a base branch
from
Open

An asynchronous version of call to shellOut #34

wants to merge 16 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Apr 14, 2018

Hi John,

Asynchronous calls looks like this.

shellOut(to: "echo", arguments: ["Hello"]) { completion in
            
	do {
	    let output = try completion()
	    XCTAssertEqual(output, "Hello")
	} catch {
	    XCTFail()
	}
    
 }      

There is a discussion here about this completion block syntax.

ShellOut.swift now only contains the public interface

Created the following for internal logic

ShellOutError.swift
ShellOutCommand.swift
String+Extensions.swift
Data+Extensions.swift
Process+Extensions.swift

ShellOutTests.swift
MARK: // Asynchronous and
MARK: // Synchronous

Original tests pushed to bottom

Looking forward to hearing your thoughts.

Sources/ShellOut.swift Outdated Show resolved Hide resolved
Sources/ShellOut.swift Outdated Show resolved Hide resolved
Sources/ShellOut.swift Outdated Show resolved Hide resolved
Sources/ShellOut.swift Outdated Show resolved Hide resolved
@iainsmith
Copy link

iainsmith commented Apr 22, 2018

Hey @rob-nash I've proposed that we drop the FileHandle API for async options in #33.

Regarding the implementation, I imagine it would be preferable to have a single async version of launchBash(...) that we can wrap in a semaphore to use in synchronous apis

Is there a reason why we need the duplication that I'm missing?

@ghost
Copy link
Author

ghost commented Apr 23, 2018

Hi @iainsmith

I was hoping to hear from John about FileHandle parameters and API style before I continued the work.

It has been a while. So I'm going to leave all of the original synchronous API in place and build the asynchronous code as an extension to the public API interface. There will be some minor duplication, as a consequence.

The sync code uses FileHandle params and the async code does not.

Travis is complaining about async tests at the moment. Those tests pass locally for me. Travis doesn't seem to recognise the 'expectation' API in XCTest framework. Sigh.

@JohnSundell
Copy link
Owner

@rob-nash @iainsmith Hey guys. Just wanted to quickly check-in here and say that I've seen these discussions and I'm planning to look into the various ideas and implementations shortly, to give you my feedback. At the moment I'm a bit busy with other projects, but I really appreciate your efforts in making ShellOut better, so hoping to merge one of these implementations in as soon as possible 👍

@ghost
Copy link
Author

ghost commented Apr 23, 2018

ShellOutTests+Linux.swift Is missing some tests. I'm not sure if this was intentional?

@ghost
Copy link
Author

ghost commented May 8, 2018

travis.yml changes as per suggestions here

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.

None yet

2 participants