Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

misleading function names in OSMImporter #231

Closed
ehx-v1 opened this issue Apr 19, 2016 · 10 comments
Closed

misleading function names in OSMImporter #231

ehx-v1 opened this issue Apr 19, 2016 · 10 comments

Comments

@ehx-v1
Copy link
Contributor

ehx-v1 commented Apr 19, 2016

on my attempt to solve #226 on my own by preparing a pull request (stopped because I lost track of the structure of OSMImporter, please you do) I discovered that isOneway(...) is actually kind of getRoadDirection(...), and the little piece of Javadoc isn't very helpful either, it should say something along the lines of

/**
 * Retrieves the direction of the given road, i.e. whether it is a one-way road from its start node,
 * a one-way road to its start node or a two-way road.
 * @param wayProperties the property map of the road
 * @return BOTH if it's a two-way road, FORWARD if it's a one-way road from the start node,
 * or BACKWARD if it's a one-way road to the start node
 */

please improve this (I'm working on Eclipse, which won't even recognize the project, it's probably coming from another IDE, but whatever the reasons I can't access the code, only attach the sources to the class files)

@craigtaverner
Copy link
Contributor

The history behind this is that OSM uses a tag 'oneway' to define this, and ways are by default not oneway, so the simplest use case is to check for this tag and return true if it exists. However, in reality, as you noticed, there is more information in this tag, and we also map from that information into direction information, so the name 'isOneway' is too simple for the actual purpose. Since you have taken the time to figure all this out, would you like to make a proposed change to the name and the javadocs? Send a pull-request and we could include it?

@ehx-v1
Copy link
Contributor Author

ehx-v1 commented Apr 25, 2016

sorry, but as I wrote, for some reason (probably a different IDE the project was created with) I can't get Eclipse to recognize the project

@ehx-v1
Copy link
Contributor Author

ehx-v1 commented Apr 25, 2016

ok I finally managed to import the project, so the pull request will be there soon

@ehx-v1
Copy link
Contributor Author

ehx-v1 commented Apr 25, 2016

pull request set off #235

@ehx-v1
Copy link
Contributor Author

ehx-v1 commented May 10, 2016

ok, now that my fork is clean I'm gonna retry

@ehx-v1
Copy link
Contributor Author

ehx-v1 commented May 10, 2016

#238

@ehx-v1
Copy link
Contributor Author

ehx-v1 commented Jun 10, 2016

Sorry! I wanted to do that on 2.3 today, but Git somehow simply won't get that it's NOT meant to apply the master branch commits to my fork's 2.3...

@ehx-v1
Copy link
Contributor Author

ehx-v1 commented Jun 10, 2016

Finally made it to create a valid pull request... The reason for the problem was that I kept re-creating the 2.3 branch locally from master. A peer then helped me fetching the remote branch, and cherry-picking the right commit. Git has nice features, but it takes time to figure out how they work right.

@ehx-v1
Copy link
Contributor Author

ehx-v1 commented Jun 10, 2016

Fun fact about this: In some article (I don't remember exactly where) I read that about... I think it was 40% of all StackOverflow questions are about Git. (This post has quite an amount of "about"s.)

@ehx-v1
Copy link
Contributor Author

ehx-v1 commented Jun 21, 2016

OK, now that my clean commit has been merged in, I think this is fixed.

@ehx-v1 ehx-v1 closed this as completed Jun 21, 2016
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

No branches or pull requests

2 participants