Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide a timeoutErrorDelay similar to FCCurrentLocationGeocoder #4

Closed
kristian opened this issue Aug 21, 2016 · 16 comments
Closed

Provide a timeoutErrorDelay similar to FCCurrentLocationGeocoder #4

kristian opened this issue Aug 21, 2016 · 16 comments
Assignees
Labels
enhancement New feature or request

Comments

@kristian
Copy link

kristian commented Aug 21, 2016

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

Fund with Polar
@fabiocaccamo
Copy link
Owner

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

@fabiocaccamo fabiocaccamo added the enhancement New feature or request label Aug 24, 2016
@kristian
Copy link
Author

@fabiocaccamo Sounds great. Thanks a ton!

@fabiocaccamo
Copy link
Owner

fabiocaccamo commented Aug 29, 2016

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

@kristian
Copy link
Author

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

@fabiocaccamo
Copy link
Owner

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

@kristian
Copy link
Author

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
Copy link
Author

kristian commented Sep 5, 2016

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

@fabiocaccamo
Copy link
Owner

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

@kristian
Copy link
Author

kristian commented Sep 7, 2016

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

@fabiocaccamo
Copy link
Owner

fabiocaccamo commented Sep 7, 2016

@kristian sure, if it is ok! :)

@kristian
Copy link
Author

kristian commented Sep 7, 2016

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

kristian pushed a commit to kristian/FCIPAddressGeocoder that referenced this issue Sep 7, 2016
@kristian
Copy link
Author

kristian commented Sep 7, 2016

@fabiocaccamo done see #5

@kristian
Copy link
Author

kristian commented Sep 7, 2016

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

@kristian
Copy link
Author

kristian commented Sep 9, 2016

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

@kristian
Copy link
Author

@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!!

@ismetanin
Copy link

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

@fabiocaccamo fabiocaccamo self-assigned this Dec 21, 2021
@fabiocaccamo fabiocaccamo closed this as not planned Won't fix, can't repro, duplicate, stale Nov 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
Archived in project
Development

No branches or pull requests

3 participants