FCIPAddressGeocoder icon indicating copy to clipboard operation
FCIPAddressGeocoder copied to clipboard

Provide a timeoutErrorDelay similar to FCCurrentLocationGeocoder

Open kristian opened this issue 7 years ago • 16 comments

Hello Fabio, thanks a lot for the library.

A minor request for improvement. At the moment you use the standard NSURLConnection connection timeout of 60 seconds. It would be favorable having a timeoutErrorDelay parameter just like in FCCurrentLocationGeocoder, because for the 4 GeoIP services you are using the total timeout sums up to 4 minutes. This is likely way to long for most applications.

Thanks & regards, Kristian

kristian avatar Aug 21 '16 18:08 kristian

Hi @kristian, you are right, that's a good idea. I will do it as soon as possible.

fabiocaccamo avatar Aug 24 '16 17:08 fabiocaccamo

@fabiocaccamo Sounds great. Thanks a ton!

kristian avatar Aug 25 '16 22:08 kristian

Hi @kristian, lets suppose that you set the global timeout to 15 seconds:

It means that if there are 5 services, each one would have a 3 seconds timeout (15/5=3) before switching to the next one, or alternatively, to avoid this case, each service would have a 15 seconds timeout, but the global timeout would be very very long (15*5=75) and it would be not clear.

Have you any idea about how to implement it in a nice and clear way?! Thanks

fabiocaccamo avatar Aug 29 '16 12:08 fabiocaccamo

Hmm, I think it's most important how you decide to name the property. Both ways are fine, if it is clear, what will happen when setting the parameter. I would avoid setting a "global" timeout, which you divide internally, for a simple reason: NSTimeInterval is an unsigned integer. If somebody decides to set a time, which is not devisable by the number of services (or the number of services changes in future), the total timeout time will not match the global timeout you specified, because of rounding errors. E.g. you have 4 services and set the total timeout to be 15s. 15/4=3.75, as an integer devision it's 3. So the total timeout will be only 4*3=12s, instead of the set 15s. And the gap will be even larger for higher timeouts. For this reason I would go for the approach to set one timeout, which is then set to each requests. As a property name I would suggest something like timeoutPerService or serviceTimeout, so it's clear that each service could take up to n-seconds. It's then up to the app developer to choose an appropriate value.

Hope this helps. Regards, Kristian

kristian avatar Aug 29 '16 18:08 kristian

I think a good solution could be to have the possibility to set both: a timeoutServiceDelay and timeoutErrorDelay.

fabiocaccamo avatar Aug 29 '16 18:08 fabiocaccamo

I don't really understand why you need two properties? What's the difference in both? Also "timeout" and "delay" are kind of the same, so I wouldn't go for timeoutServiceDelay. Rather I would go for the iOS default pattern, they call the property timeoutInterval so what about serviceTimeoutInterval or timeoutIntervalPerService.

kristian avatar Aug 30 '16 20:08 kristian

@fabiocaccamo Any update on this? This would be highly anticipated. Thanks a lot! 👍

kristian avatar Sep 05 '16 18:09 kristian

@kristian I'm sorry, but I don't think I will have the time to do it this month...

fabiocaccamo avatar Sep 06 '16 10:09 fabiocaccamo

@fabiocaccamo Would you accept a pull request then, if I would provide you with one? :)

kristian avatar Sep 07 '16 08:09 kristian

@kristian sure, if it is ok! :)

fabiocaccamo avatar Sep 07 '16 08:09 fabiocaccamo

Sure, let me check back this afternoon. If I find time I'll provide you with a pull request.

kristian avatar Sep 07 '16 09:09 kristian

@fabiocaccamo done see #5

kristian avatar Sep 07 '16 15:09 kristian

PS: I didn't change the podspec in the pull request. Please go ahead and adapt the podspec if possible. Thanks.

kristian avatar Sep 07 '16 15:09 kristian

@fabiocaccamo Would be great if you find some time to merge the change this weekend! 👍 Thanks a lot in advance.

kristian avatar Sep 09 '16 18:09 kristian

@fabiocaccamo Any news on merging #5? :-) Higly appreciate it, sorry for insisting so hard, but it would be great, if I havn't have to add this single dependency manually. Thanks!!

kristian avatar Sep 18 '16 17:09 kristian

@fabiocaccamo @kristian Hi! Do you have any news about merge this issue?

ismetanin avatar Feb 16 '17 13:02 ismetanin