Closed cojoj closed 9 years ago
@czechboy0 I've changed/repaired thing you've pointed so... Your job now to review it one more time. I'm still pretty skeptical because we don't have a proper test coverage to check of everything is working properly.
@cojoj The lack of tests is definitely something to fix sooner rather than later. But up until now the only thing I could have done was to not release the project until I have time to write them.
I would definitely appreciate if you started adding some tests in the suite.
I think you missed the issues in HTTPUtils.swift
, I think there are some logic problems with guard
(see inline comments).
@czechboy0 completion
you're talking about should return only NSError
? Of what type?
@cojoj What I mean that in the else
block, you should call something like completion(response: nil, body:nil, error:Error.withInfo("Could't create request"))
and then return
.
@czechboy0 IMHO we should drop returning NSError
s and switch to throw
ing them but it's probably a bigger portion of refactoring.
Cool, now just please fix the logic issues in HTTPUtils.swift
and we'll be able to merge :)
I've changed them and reverted them back... Deserve some kind of award 😓
Awesome!
You get a :fireworks:
:clap: Thanks!
Weeeeeeee!!! 🎆
Can you also please re-apply the changes in https://github.com/czechboy0/XcodeServerSDK/commit/41bfb8a3f3b0df6a58aa0a2fc722c6091207781e? Because you moved the
XcodeServerConfig
into a separate file, so the merge won't apply that cleanup change anymore (you'll have to do it manually).