-
Notifications
You must be signed in to change notification settings - Fork 921
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
Support version for /nodes, /ways, /relations #1189
Conversation
7f7da67
to
ea747e8
Compare
ea747e8
to
b7eec59
Compare
Are we actually taking API extensions like this in the current API version? or in the rails code at all come to that? shouldn't anything new be in cgimap? |
I'm not sure if it's a good idea.
Yes, I think we want these in cgimap |
I suspect that we have a lot of rails port instances out there (and if it is just for testing) that don't run cgimap, requiring it (at least before the Siamese twins have been cleanly separated) at this point in time would seem to be slightly counterproductive. |
Sorry, I'm late to the discussion here... First, this looks great! I'd be in favour of something like this being merged. It would be great to have it in cgimap too, especially if it's expected to be a high-traffic call. Any help with that would be very welcome. However, I don't think whether it's in cgimap should delay merging it to rails now. It looks like the format |
@zerebubuth yes it probably would but then we would need it in cgimap because I think those endpoints are already handled by cgimap in production? |
That's a good point. Although I feel like it is more elegant to keep them in the same API call, it seems that it would be more practical to keep them separate. |
I must respectfully disagree. We shouldn't compromise the design just so we can deploy it quicker. Especially not for something like the API that then has to hang around for ever. |
I agree with Matt that adding |
To echo support for this, I'd use it in iD if it were available in the API. |
👍 to the existing calls being the right place to add it. I'm still undecided on if this should be added in 0.6 |
I've started (very preliminary) work on this on a cgimap branch. So preliminary, it currently breaks the tests! But hopefully should have something workable soon-ish, and we can sync the implementation behaviours and push them out. If anyone would like to help, I'd really appreciate it. Code, documentation, "unit" tests, more eyes on existing PRs - anything would be welcome. |
Apparently cgimap supports multiple version fetches by now, at least http://www.openstreetmap.org/api/0.6/nodes?nodes=421586779v1,421586779v2 seems to be working as described earlier in this issue. Unfortunately, this new syntax is not yet described on the API v0.6 Wiki page. "Multi fetch: GET /api/0.6/[nodes|ways|relations]?#parameters" might be a good place for this. Otherwise, this issue could probably be closed then? |
@mmd-osm Have you checked if a corresponding rails implementation exists? As this PR was never merged I assume not. |
Exactly - until this code base supports it I don't think this should be closed. |
Updated documentation in now available on the wiki page: https://wiki.openstreetmap.org/w/index.php?title=API_v0.6&type=revision&diff=1581158&oldid=1571641 Please feel free to enhance the description as needed. |
Closing because #3715 supersedes this PR. |
Fixes #1067.
Adds the following API methods:
/api/0.6/nodes/versions?nodes=123v4,56v7
/api/0.6/ways/versions?ways=123v4,56v7
/api/0.6/relations/versions?relations=123v4,56v7