search

geocoder-php / Geocoder

The most featured Geocoder library written in PHP.
https://geocoder-php.org
MIT License
3.95k stars 516 forks source link

MapQuest improvement #840

Closed TerjeBr closed 6 years ago

TerjeBr commented 6 years ago

This is an improvment to the MapQuest provider, that we talked about in #839

I have tried to stay away from the code in the Common directory, but I could not avoid some small minor changes there to make it all fit together. Hope this will be ok.

Nyholm commented 6 years ago

Thank you for this PR. I did a quick review and I see some issues when using a chain provider. Do you really need a custom implementation of a query?

TerjeBr commented 6 years ago

The custom implementation of a query is only to make it easier to use. It will work fine with a reqular query as well, as I wrote in the Readme.md

Nyholm commented 6 years ago

I would be way happier without the custom implementation. That would also avoid the changes in Common

TerjeBr commented 6 years ago

I am sorry that you do not like the custom implementation of GeocodeQuery. Is there something in particular that bothers you about it? Something I can improve?

Or are you just opposed to having two GeocodeQuery implementations in general?

The additional one should work fine as a replacement for all the cases where the original one may be used.

Without the new GeocodeQuery implementation, you first have to create a GeocodeQuery with a dummy address string (it cannot be empty), and then clone it in order to have the address data on it. I found this to be a bit cumbersome, so that is why I added the new implementation.

TerjeBr commented 6 years ago

It is a bit unfortunate that the interface Geocoder\Provider\Provider (that all the providers must implement) depends on the concrete classes Geocoder\Query\GeocodeQuery and Geocoder\Query\ReverseQuery instead of interfaces. When in addition those concrete classes are final it leaves no room for changing or improving the Query classes that are used in a graceful way.

I guess changing it to use interfaces is the "right" thing to do, but that will be a huge BC break. Removing the final keyword on the concrete classes is less invasive. If you choose to do that, you also might as well make the properties protected instead of private.

The only other alternative I can see is to declare that the Query classes are set in stone, and we can have no suggested changes or development on the Query classes. I think that would be a bit unfortunate thing to do.

TerjeBr commented 6 years ago

Hi @Nyholm what/when will be the next step?

TerjeBr commented 6 years ago

Thank you for your review. I will wait for your response before I push a new set of changes.

TerjeBr commented 6 years ago

@Nyholm any updates on this? I am still waiting for your comments.

TerjeBr commented 6 years ago

@Nyholm Have you been on vacation or something?

Please continue the conversation so we can land this PR.

TerjeBr commented 6 years ago

@Nyholm if we are going to change the Common\Query\GeocodeQuery class from final to non-final, do you not agree that it makes sense to also change the constructor and all the properties from private to protected?

Do you agree with the above statement? Or do you have any reason I am not aware of for wanting to keep it all as private?

TerjeBr commented 6 years ago

Ok, If you will please accept #849 that I have made with the minimal changes to the common files, I will then after that make the necessary changes to this PR to reflect that.

Hopefully we can then move this PR forward.

TerjeBr commented 6 years ago

@Nyholm for me to make the tests pass you need to tag a new version of https://github.com/geocoder-php/provider-integration-tests

TerjeBr commented 6 years ago

@nyholm plase make a new version tag on the geocoder-php/provider-integration-tests repository so that I can update the composer.lock files so this will do the tests properly.

You will also need to merge #849 and add a tag to that before this PR can show green on all tests.

TerjeBr commented 6 years ago

@Nyholm, have you gone on vacation again?

TerjeBr commented 6 years ago

@Nyholm Are you celebrating Norways national day today? Is that why you are not doing anything with this PR today?

TerjeBr commented 6 years ago

@Nyholm ping

willdurand commented 6 years ago

I am locking this PR until we have a chance to review it. I am sorry but there is no need to ping/comment every day, that won't speed up the process.

Nyholm commented 6 years ago

Im sorry Terje for not replying as quickly as you would like me to.

When I said:

Try you restrict yourself to not update the common objects. You may send another PR with these changes later.

I meant: Please update this PR so you do not change common objects.

I did not mean: Make a new PR with changes in common to be merged before this one.

The common files has over 30 different providers. They all are happy with the current abstraction. Im sure that even the MapQuest could find a way to use them in the current state.

For a future PR (after this one is merged), Im happy to review changes in common that you can prove would significantly improve maintainability and user experience for at least a handful of providers.

TerjeBr commented 6 years ago

I have complied with everything you have requested, as far as I know.

If you want to merge just this pull request, that is fine, then you can ignore #849 Or if you want do do it in stages, you can merge #849 first, and then this one.

Please tell me if there is anything else you need for this to be merged.

Nyholm commented 6 years ago

Please tell me if there is anything else you need for this to be merged.

I'll try to be extra clear with what I mean.

This is the process I want to follow: 1) You update this PR so you do not modify any classes in common 2) I review it and after I'm happy I'll merge it 3) (optional) You make a PR with changes in common that you can prove would significantly improve maintainability and user experience for at least a handful of providers.

TerjeBr commented 6 years ago

Does that mean you are backtracking on what we already have discussed?

On 13th of April you agreed that I could at least change the Common\Query\GeocodeQuery by removing the final keyword. All the other changes I can code around, but I cannot code around that.

Less important are the changes to Http\Provider\AbstractHttpProvider (you even thanked me for adding a function there) and a small bugfix in Common\Model\AddressBuilder. That is all the changes that you will find in #849

So, have you changed your stance so to not allow any changes at all in the common classes now?

TerjeBr commented 6 years ago

And again we have this great wall of silence.

Is it too much to expect that you are able to respond to me within a day or so? Does it have to be weeks between when we exchange comments on this PR? I find that very frustrating.

Nyholm commented 6 years ago

And again we have this great wall of silence.

Im doing open source work on my free time because I enjoy it. Doing open source is however not the only thing Im doing before and after normal work. Im not sure if I intrepid your messages correctly, but I sense a hostile tone. (Correct me if Im wrong.) That makes it no fun to spend time helping you get your PR merged.

So, have you changed your stance so to not allow any changes at all in the common classes now?

I've not changed my mind. But I feel that you do not really listen when I say "restrict yourself from making changes in common." I know it is possible to add the features you want to add without any changes at all in common (as explained). I want you to see that as well. Since I want to help you to get this merged, I now ask you to not make any changes at all in common since that will ease reviewing and the decisions to merge is not too critical for the rest of the project.

You may think Im unfair or unreasonable. I do not want to discuss this anymore, but Im happy to review an updated PR. If other maintainers think do not agree with me you are more welcome to say so.

TerjeBr commented 6 years ago

I know it is possible to add the features you want to add without any changes at all in common (as explained).

This is not true.

It is impossible for me to continue without at least removing the final keyword from the Common\Query\GeocodeQuery class. I thought we already discussed that, and that you agreed to that change.

All the other ones I can code around, but not that one.

TerjeBr commented 6 years ago

Shall I go ahead and make the PR so that the removing of the final keyword is the only change among the common files?

TerjeBr commented 6 years ago

You are aware that I updated this PR on 14th of May right? And that that update made it only contain 3 small changes to the common files that I thought we were in agreement on?

If you were not aware of that, that may be the cause that we are talking past each other?

TerjeBr commented 6 years ago

@Nyholm please answer me so I can go ahead with this PR

TerjeBr commented 6 years ago

Since you do not give any answers, I will just have to assume. I assume then that I reached the correct conclusion when I said "make the PR so that the removing of the final keyword is the only change among the common files". I will go ahead and do just that.

TerjeBr commented 6 years ago

Ok now everything should be as you want it.

Please tag a new version for https://github.com/geocoder-php/provider-integration-tests if you want all the tests to pass before you merge.

unkind commented 6 years ago

Without the new GeocodeQuery implementation, you first have to create a GeocodeQuery with a dummy address string (it cannot be empty), and then clone it in order to have the address data on it. I found this to be a bit cumbersome, so that is why I added the new implementation.

Do I understand right that you want to remove final just because you're missing named constructor like GeocodeQuery::location(...)?

Maybe it's better to improve GeocodeQuery model instead of inheriting it?

unkind commented 6 years ago

@Nyholm argh, removing final is a very bad sign in most cases. You can call me next time :)

unkind commented 6 years ago

After reading original topic, I think you can just make (static) function to create GeocodeQuery for MapQuest: MapQuest::createQuery(...).

By the way, why does Geocoder have Query interface? I thought you decided you throw it off: https://github.com/geocoder-php/Geocoder/pull/583/commits/40bdb8d9d78a52e88c95b5856b7ee9a216e476d1

Nyholm commented 6 years ago

I've added a PR to your PR: https://github.com/TerjeBr/Geocoder/pull/1

TerjeBr commented 6 years ago

Ok, I am back from my vacation. Let's see what has happened here.

TerjeBr commented 6 years ago

@unkind wrote:

Do I understand right that you want to remove final just because you're missing named constructor like GeocodeQuery::location(...)?

Maybe it's better to improve GeocodeQuery model instead of inheriting it?

You are right that it would be better to improve GeocodeQuery instead of inheriting it. That is what I asked about first in #839, if there could be a way to incorporate the address in the query. But that seems to be a no go from @Nyholm.

So, this quest of mine to inherit from GeocodeQuery is my attempt to circumvent that so mapquest users could have at least some level of user friendliness in the use of this liberary. But now that hope seems to be thwarted too, since he has obviously changed his mind and wont even allow the final keyword to be removed from the class (that he previously agreed to).

TerjeBr commented 6 years ago

Ok, I hope this can finally be merged now then.

It fails the style guide check, but that was in the changes that @Nyholm introduced, so I guess that is ok.

TerjeBr commented 6 years ago

Ping