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

Add Reactive events to CLGeocoder #37

Closed
wants to merge 6 commits into from

Conversation

lordzsolt
Copy link
Member

@lordzsolt lordzsolt commented Sep 18, 2020

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.

@bobgodwinx
Copy link
Member

@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

@lordzsolt
Copy link
Member Author

lordzsolt commented Oct 4, 2020

@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: Github Enterprise: "context.github.repos.getContents is not a function" repository-settings/app#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.

@bobgodwinx
Copy link
Member

bobgodwinx commented Oct 6, 2020

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

@lordzsolt
Copy link
Member Author

lordzsolt commented Oct 7, 2020

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 lordzsolt closed this Jul 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Retrieve wrapper around CLGeocoder?
2 participants