Refactored OSMImporter.isOneway() to OSMImporter.getRoadDirection(), … #238
Conversation
|
The problem about Travis CI is probably exactly what I noticed, it has the old method name while post-commit Neo4j Spatial only has the new one. |
isOneway(...) seems to be needed in references like e.g. Travis CI, so I re-added it as redelegation to getRoadDirection(...), as the implementation stayed the same (the new Javadoc links towards getDirection(...) too)
|
Added the old name as redelegation. Javadoc states the method as "legacy" and links towards the Javadoc of the refactored method. |
|
Have you signed the CLA? If not, please check the instructions at http://neo4j.com/developer/cla/ |
|
Just done. Already wondered where the instructions are - they were in the wiki for any pull requests where I found a mention, and the wiki does not exist at the moment. |
|
I wonder what's up with Travis CI though... |
| */ | ||
| public static RoadDirection isOneway(Map<String, Object> wayProperties) { | ||
| return getRoadDirection(wayProperties); | ||
| } | ||
|
|
There was a problem hiding this comment.
I'm not sure if isOneway(...) should perhaps be marked as deprecated, but I think it should not.
There was a problem hiding this comment.
Since this code is very likely only used internally, despite this being a public method, I have no objection removing this method. Also, all of Neo4j Spatial is alpha code, and changing the API is completely acceptable at this point. Should there be users that are relying on this we can either revert that decision, or support them changing to the new API.
|
The problem with Travis is that it is not accepting Java8, which we are using in master (since Neo4j 3.0 has moved to Java8). I personally think you should fix this at least on Neo4j 2.3 version (like the branch 0.15-neo4j-2.3). You could consider even fixing it on 2.2, but the further back you go the less value. Between 2.3 and 3.0 there were big changes, so it makes sense to support one version before those changes. The process I suggest is:
Then travis CI will probably be able to compile and test this new PR and I can take it from there:
For future reference, I view 'master' as where new development happens. All maintanance work should happen in older branches and merged forward. This current PR is kind of in the grey area, could be maintenance, could be cleanup. I'm voting to do it on an older branch because we get around travis CI. |
|
Alright, moving the change to 0.15 then. |
|
That means, if I still get to it, because I'm quite busy with the actual company product at the moment. |
…changed Javadoc.
My proposed solution to #231.
Perhaps a new method isOneway() redelegating to getRoadDirection() should be added though, for those using the method from outside. Because references outside of Neo4j Spatial most probably won't be auto-updated.
Unfortunately, I thought of that too late...