FileHandle.read() should have a discardable result

[Ponyboy47]

In #54 The fix to #52 was to call .read() on the pipes before calling .finish(). When manually managing an AsyncCommand, the fixit is to call .read() "even if you're not going to use the result". I think that this should warrant the .read() function being marked with the @discardableResult attribute.

Currently in my code, before calling command.exitcode() I have a line that looks like this:

command.stdout.read()

This line generates a warning when building.

I'm unsure if the readSome(), readData(), and readSomeData() variants should be marked as @discardableResult as well.

[kareman]

You can silence the warning like this:

_ = command.stdout.read()

[Ponyboy47]

I realize that I can do that. I could also just continue to ignore it since it doesn't break anything. ;)

I don't want to just silence/ignore warnings though. If a return value can be reasonably expected not to be used, like the result of .read(), then it should be marked with @discardableResult. This allows for cleaner code overall and follows community accepted best-practices.

There are numerous examples of other frameworks and blog posts recommending that instead of doing:

_ = command.stdout.read()

like you're suggesting, to just mark the method with @discardableResult.

References: Blog post describing the @discardableResult addition in Swift 3 and how one of its primary uses can be to clean up _ = func() into just func(). SQLite.swift commit where they removed tons of _ = func() in favor of using @discardableResult and just func().

It's always better to fix things that cause compiler warnings at the source rather than force developers to work around them. If you're saying that in order for this framework to work, we HAVE to call .read(), but that we are NOT required to use the result of it, then you should mark it with @discardableResult.

I'm happy to make the change myself and submit a pull request, but I would like your input on whether you think that all of the read variants should also be marked.

[kareman]

Yes I see your point. But I referred to the wrong method in that issue. I should have said readData, because read will also convert the data to a String. So you can mark Stream.readData as discardable. I don’t think any of the others should be marked, as there is no point in calling them without using the result.

[Ponyboy47]

Ahh, sorry for the confusion/rant. I will update my code to use .readData() instead, and I have submitted the pull request

[kareman]

No problem, it's good to have users that care about the usability of the framework. And thank you for your pull request, I just merged it.