Resume alamofire request only and only if it hasn't been started previously

[piv199]

fixes #163

Tested in the example with starts immediately true || false, now it's working as expected... So no second call for alamofire request resume method.

[pcantrell]

Awesome! Thank you for taking this on.

I’ll add a couple of minor project style consistency things in the review notes.

It would be great to get regression tests for this. It’s subtle enough — and easy enough to mess up — that it’s worth covering.

I take it that if alamofireRequest.task, then resume() is the correct action to take? If I’m following that correctly, your ?? solution is a nice bit of concise ingenuity. When does nil task even happen, and how is it different from suspended? This question has a bearing on how the specs would be structured.

[pcantrell]

This project never uses superfluous self.

[pcantrell]

I know the brace & indentation conventions of the project are weird, but please do follow them. Brace rules are at the end of the README, or you can just look around the project for examples.

[piv199]

It would be great to get regression tests for this. It’s subtle enough — and easy enough to mess up — that it’s worth covering.

I' not familiar with tests you have, also I haven't found siesta+alamofire tests at all...

When does nil task even happen, and how is it different from suspended?

As for Alamofire sources, task should not be nil. When we initializing the Request object we pass there a task (that could be nil). So possibly, if something went wrong while creating a URLSessionTask object than nil will be passed to Alamofire.Request. Actually resume has guard for checking on task existense - it means that we could not resume task that is nil.

[pcantrell]

I haven't found siesta+alamofire tests at all...

The existing tests rerun a large subset of the tests serval times on CI using four different networking provider/setting combinations, making sure that all the different underlying providers result in the same Siesta behavior.

We could add a fifth provider to that suite with startRequestsImmediately = false, and maybe that would be a good sanity check, but that doesn’t actually catch the original problem your PR fixes! The reason is that the existing strategy only tests Siesta behavior via the provider, but the bug you found affects underlying Alamofire behavior not exposed through Siesta. So this bug calls for a new regression test in a new file.

I' not familiar with tests you have

If you give me push access to your pull request, I can drop the test in the right place.

[piv199]

@pcantrell This option is selected

• Allow edits from maintainers.

[pcantrell]

I dug into this a bit, and decided that the substantial amount of code it would take to test the fix simply isn’t justified. Sorry for the long delay on accepting this, @piv199, and thanks again so much for chasing the issue down!

Did you file an issue with Alamofire about the redundant notification? It really does seem like that’s a bug.