RxCoreLocation icon indicating copy to clipboard operation
RxCoreLocation copied to clipboard

Add Reactive events to CLGeocoder

Open lordzsolt opened this issue 4 years ago • 4 comments

Fixes #36

I'm unsure of a few things:

  1. How to update the non-existent changelog 😄
  2. How to add tests
  3. Should we forward the error received inside the CLGeocodeCompletionHandler? It was not done before, however all methods on CLGeocoder have the following notice:
After initiating a forward-geocoding request, do not attempt to initiate another forward- or reverse-geocoding request. Geocoding requests are rate-limited for each app, so making too many requests in a short period of time may cause some of the requests to fail. When the maximum rate is exceeded, the geocoder passes an error object with the value CLError.Code.network to your completion handler.

lordzsolt avatar Sep 18 '20 15:09 lordzsolt

@lordzsolt

  1. It looks like danger now requires the change log. You can add a CHANGELOG.md file detailing your changes in this MR. Please also add it to the Readme.md
  2. For tests you can see examples in RxCoreLocationSpec we use quick and nimble for testing
  3. Yes it would be nice to forward the error from CLGeocodeCompletionHandler

bobgodwinx avatar Sep 19 '20 11:09 bobgodwinx

@bobgodwinx Updated the PR.

  1. ~~Added a CHANGELOG.md file, however I'm unsure if that is the problem. A quick googling lead me to this issue: https://github.com/probot/settings/issues/123 I am not sure if it's related, but based on that, I would assume danger.github.api.repos.getContent needs to be changed to danger.github.api.repos.getContents, since the getContent function got removed. It seems to be coming from here: https://github.com/RxSwiftCommunity/peril and it's run on every PR for repositories under RxSwiftCommunity. I could not find any PRs that passed the check :|~~

Yep, that was the problem. It got fixed.

  1. Yes, I noticed Quick and Nimble are being used. However, the current tests have no asynchronous behavior by the looks of it. I would need to mock out the base CLLocation. Or should I add some toEventually behavior?

  2. Added.

lordzsolt avatar Oct 04 '20 19:10 lordzsolt

@lordzsolt Yes let's add toEventually for testing that would also work. I would test if during the weekend.

bobgodwinx avatar Oct 06 '20 11:10 bobgodwinx

Added tests using toEventually, however I was getting the following error when running the tests: MobileGestalt.c:1647: Could not retrieve region info

Google suggested it's because it's ran on a simulator rather than real device. Using the .gpx file is supposed to fix it, however I didn't manage to get it working.

If you have any ideas how to fix it, I would appreciate some suggestions :)

lordzsolt avatar Oct 07 '20 22:10 lordzsolt