Stop calling LatLong stuff GeoCoordinate #23
Conversation
Geo.php
Outdated
| @@ -15,7 +15,7 @@ | |||
| return 1; | |||
| } | |||
|
|
|||
| define( 'DATAVALUES_GEO_VERSION', '1.1' ); | |||
| define( 'DATAVALUES_GEO_VERSION', '2.0' ); | |||
There was a problem hiding this comment.
Unless you intent this thing to be tagged right after this commit, this should be 2.0 alpha
|
This change is rather annoying for the Maps extension. Second time it needs to go update a bunch of class refs in a breaking manner, for reasons simply not relevant to it. It's also annoying for the WB code using this component. Such code will work either with 1.x or 2.x, but never both. So you won't just have the updating work from 1.x to 2.x, you'll also end up with components that have not done this yet no longer being able to use the latest versions of those who did. The change is a nice cleanup, but probably not worth all this hassle on its own. How about we keep it under the |
|
I had the impression that the planned alias drop in 2.0 will break enough stuff anyway. Therefor my suggestion to do this rename in the same release. But I don't mind. It's fine for me to do this later in an other release. |
That is my suggestion as well. What I'm saying is that with just these things, I do not think it makes sense to already make such a release. |
|
Don't we need an alias-free release for HHVM soon? |
|
Sure we do [*]. What do you think that means doing here though? Do you think it makes this commit needed? Or is some other thing that will also be a breaking change needed? [*] A release that does not use any aliases - having the aliases themselves is not a problem, as long as our code does not use them |
|
Three actual renames are happening in this patch:
I added deprecated alias. |
| */ | ||
| class GeoCoordinateParser extends \DataValues\Geo\Parsers\LatLongParser {} | ||
|
|
||
| } |
There was a problem hiding this comment.
As I understood this is only for IDE.
If so it is not needed, as soon as PhpStorm supports class_alias nowadays. See https://blog.jetbrains.com/phpstorm/2016/11/phpstorm-2016-3-rc1/
There was a problem hiding this comment.
This is correct. This is for older versions. Note that the Aliases.php file was already there and I did not wanted to have it out of sync with the actual aliases. We might remove this file entirely some day.
There was a problem hiding this comment.
What about other major PHP IDE's and tools? I'd not drop this if it's just PhpStorm that supports class_alias
This always bugged my. "GeoCoordinate" was the old name of the value class. It was renamed a long time ago. For some reason parser and formatter were never renamed. Why not?
This is, obviously, a breaking change.
I suggest to do this along with the "kill all aliases" patch.