geocoder-php / Geocoder

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

No interface for formatters? #843

Open burzum opened 6 years ago

burzum commented 6 years ago

I'm building a wrapper for this library and a service class for my app and when I wanted to make the formatter exchangeable I realized that I can't typehint the setter against an interface.

https://github.com/geocoder-php/Geocoder/blob/master/src/Common/Formatter/StringFormatter.php

If you agree I'll create a PR and add a Formatter interface that implements the format() method.

Would \Geocoder\Formatter\FormatterInterface.php work for you?

Nyholm commented 6 years ago

Hm. Are the StringFormatter ever referenced anywhere? Also it seams like we only have e one formatter... I think you are correct. We need an interface for this.

burzum commented 6 years ago

I'm not sure. :) We're already using some other lib for address formatting but it's ancient AFAIR, I would have to look it up, might do it later. But I would prefer to use one package and geocoder seems to be nice and modern.

It is very likely that we'll have to implement our own address formatters because we feature 18 languages in translations and even more without but the editors and our clients are a PITA when it comes to stuff like addresses and I18n date and times... I had to implement our own date formatting because they always came up with some non standard wishes... especially in Japanese.

willdurand commented 6 years ago

The rationale behind not adding an interface is that there is no point into having different formatters with the following signature format(Location $location, string $format): string, a string formatter is a string formatter, and we wont change it since it relies on placeholders.

Nyholm commented 6 years ago

Thank you. You are very much correct. But we might use a different signature.

Say “format(Location $l)”. That would allow us to have JsonFormatter or MyCustomObjectFormatter.

burzum commented 6 years ago

@willdurand what if the config is passed as first arg (array) into the constructor? This would allow you to configure any formatter the same way.

willdurand commented 6 years ago

@willdurand what if the config is passed as first arg (array) into the constructor? This would allow you to configure any formatter the same way.

that would not work, see this formatter like printf()

burzum commented 6 years ago

Why wouldn't this work? If we merge it with a default array it would be always present.

return strtr($this->config['format'], $replace);

The 2nd arg of format() could become an array as well and then simply:

return strtr($config['format'], $replace);