Add a method to return DataResponse from DataRequest.responseString()

[philippelatulippe]

This is similar to PR #79 except for responseString() instead of responseJSON(). I wrote it to work identically to the code in that pull request.

It allows access to metrics (NSURLSessionTaskMetrics mentioned in issue #76 ) for those who need the response as a string, not JSON.

[jacobh0]

Can we merge this?

[freak4pc]

I'm not entirely sure we'd want to overload responseString this way. responseJSON only has one meaning - and responseString will now have an overload returning two very different variations.

@sunshinejr Any chance you have thoughts on this?

[sunshinejr]

Hm, I'm not sure about this one. How about making it a dataResponse? Personally, I wouldn't expect that DataResponse<String>would be the return type of responseString function.

[jacobh0]

Ended up ditching RxAlamofire as its become rather out of date and surfaces no data operations. Just wrapped this response method in an observable manually and saved a Framework import.

[freak4pc]

If the op of this PR wants to go with dataResponse I think we'll be OK with that. Glad you were able to solve your problem - Indeed if you only have a very narrow use case, using a library (for anything, really) might not be needed.

[philippelatulippe]

Since the merge of PR #79, responseJSON does in fact have two meanings. It can return an observable of (HTTPURLResponse, Data) or an observable of DataResponse<Any>. In my merge request I copied this format exactly. But I agree that the naming isn't clear.

Should I rename both of them to dataResponseJSON and dataResponseString?

A further issue with both of these methods is that they don't return the DataResponse on error, which is important for the main use-case (metrics and error logging). I'm not sure how to implement this though: wrap the error and the DataResponse into a new error type, or call onNext even on error and let the consumer sort it out.

[philippelatulippe]

I have rebased this branch on top of master, and I've also updated the function names to distinguish them from the regular response functions. I would love to see this merged. :)

As an additional feature, the observables returned by these functions could also return the DataResponse on error. I'm not sure how to go about doing this. Would it be OK to emit the DataResponse as an error instead of the error?

The following code:

if let error = response.result.error {
      observer.on(.error(error))
}

would become:

if let _ = response.result.error {
      observer.on(response)
}

Subscribers would then need to extract the error themselves.

Alternatively, I could create a new error case that contains both the error and the response, or I could emit a tuple with both the error and the data response.

[freak4pc]

Hey @philippelatulippe - we follow semantic versioning, we can't make breaking changes like you did to the API without bumping a major version, and I wouldn't want to bump a major version for something to minor.

[philippelatulippe]

Thanks for responding so quick, it wasn't my intention to make a breaking change to the API, renaming responseJSON was a mistake.

[philippelatulippe]

I've pushed a change to my commit which only introduces a new function, dataResponseString.

[freak4pc]

Looks good with a few tiny notes.
Also wondering if it might be better as a Single but I don't mind too much.

[freak4pc]

Suggested change

	- returns: An instance of `Observable<SDataResponse<String>>`
	- returns: An instance of `Observable<DataResponse<String>>`

[freak4pc]

Suggested change

	Returns an `Observable` of a DataResponse<String> for the current request.
	Returns an `Observable` of a `DataResponse<String>` for the current request.

[freak4pc]

Would appreciate a second set of eyes if you have a minute @sunshinejr :)

[philippelatulippe]

I pushed the changes based on the review suggestions. The tests seem to have failed because someone registered http://401.xyz.

[rynecheow]

@philippelatulippe able to resolve the conflicts with master branch and update the PR?