Skip to content

Conversation

@mrkrstphr
Copy link

This is by no means a complete implementation as I have only handled GoogleMaps, although the other providers should still work fine.

I have a use case for retrieving multiple results from Google and displaying them to the user to pick the correct one. This library only returns the first result, though.

I have modified the code to return all results from GoogleMaps, returning an array if there is more than one or a Result if there is only one.

Maybe for backwards compatibility, this could be controlled with some kind of config toggle. This pull request isn't meant to be accepted straight out, but to illustrate what I desire to achieve and how it could be achieved. Thoughts? Ideas?

@willdurand
Copy link
Member

Well, we need to find a better solution (BC) to handle this use case, because I think it can be useful.
ping @toin0u @Baachi

Maybe having a second parameter in the geocode() could do the job, like $allResults = true|false (not sure though)

@mrkrstphr
Copy link
Author

Yeah, not sure why I didn't just go that route. Also, I think I botched my commit for this pull request. I made it against a tagged version, then copied it in for master and might have messed things up. Feel free to ignore the pull request, but please consider the idea :-) If you want me to come up with something with a toggle for it, I'd be glad to contribute.

@toin0u
Copy link
Member

toin0u commented Apr 17, 2013

I think it's a good idea to change the geocode() signature with a toggle which will be set false by default. 👍

I wonder how many providers return more than one result though..

@Baachi
Copy link
Member

Baachi commented Apr 17, 2013

We can add a new method in the Result class, named getSimiliarPlaces which return the other results. This would not be a BC break.

@mrkrstphr
Copy link
Author

How would you maintain state between the two calls (gecode() and getSimilarPlaces())? Would it end up making the API call twice (which could cause rate limiting issues for providers that limit calls per period)? Or are you saying getSimilarPlaces() would be an alternative to calling geocode()?

@willdurand
Copy link
Member

Well I guess the geocode() method would hydrate the main Result as well as the similar ones if available.
That would imply a collection of Result in the Result class.

I think extending the geocode() method is better as you explicitely ask for a result set, or a single result. You don't have any difference between the first result, and the last one for instance.

@Baachi
Copy link
Member

Baachi commented Apr 18, 2013

We can provide two differnet ResultFactorys. The first ResultFactory hydrate only the first element and the other factory hydrate all entries. But we must change the return value from each provider, to support multiple results.

Simple implementation: https://gist.github.com/Baachi/ea8f67e2f5329515c2c3

@willdurand What do you think?

@willdurand
Copy link
Member

@Baachi well, it looks good.

@toin0u
Copy link
Member

toin0u commented Apr 18, 2013

@Baachi good idea 👍

toin0u added a commit to toin0u/Geocoder that referenced this pull request May 2, 2013
toin0u added a commit to toin0u/Geocoder that referenced this pull request May 3, 2013
toin0u added a commit to toin0u/Geocoder that referenced this pull request May 3, 2013
@willdurand willdurand closed this in ca92c26 May 3, 2013
willdurand added a commit that referenced this pull request May 3, 2013
Added: DefaultResultFactory and MultipleResultFactory classes - Fix #223
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.

4 participants