search

PromiseKit / CoreLocation

Promises for Swift & ObjC
http://promisekit.org
MIT License
30 stars 29 forks source link

Request a single location as described in the documentation #9

Closed mrgrauel closed 6 years ago

mrgrauel commented 6 years ago
mrgrauel commented 6 years ago

Does it have to be Swift 3 compatible? @mxcl

mrgrauel commented 6 years ago

Just a note I am on vacation till next Saturday. Feel free to adjust yourself 🚀

mxcl commented 6 years ago

I use this code in 4 apps in the store and it works fine, what is the justification for these changes?

mxcl commented 6 years ago

This diff is also not readable, you change the indentation for everything in the file, if I am to understand the changes you'll have to avoid that, thanks.

mxcl commented 6 years ago

Closing due to lack of response and that I can't figure out what this is for or what the diff does due to excessive whitespace changes.

mrgrauel commented 6 years ago

Sorry for the lack of response. As I mentions I have been on vacation till Saturday.

Isn't it like it that the LocationManager always uses the first reported location and doesn't keep searching until the satisfying block is fulfilled? It always instantly returns cause startUpdatingLocation() instantly returns a location to the delegate.

The documentation of the extension says that it requests a single location. Shouldn't we wait for the locationManager to determine the needed criteria and use the intended method to request a single location requestLocation()?

mrgrauel commented 6 years ago

I changed the indentation sorry for the problem.

mxcl commented 6 years ago

This needs rebasing, and still contains noisy whitespace changes.

I am afraid I also still do not understand your justification for this patch. You are saying that we should use the requestLocation function Apple provides, but why? I see no concrete rationale for for why in Apple's documentation.

The other issue you are talking about is fixed, hence I believe you need to rebase.

In future if your PR fixes two things please submit two PRs since otherwise it is impossible to properly review the changes independently.

mrgrauel commented 6 years ago

The current master works now as expected. As you said the bug is fixed 👍