roelderickx / ogr2osm

A tool for converting ogr-readable files like shapefiles into .pbf or .osm data
https://pypi.org/project/ogr2osm/
MIT License
59 stars 14 forks source link

Document --is-positive #48

Closed citizenfish closed 7 months ago

citizenfish commented 8 months ago

This one caused me a lot of headaches only cured by a dive into the code

Please can you document the command line parameter --is-positive and the fact that --idfile and --saveid count DOWN by default. This will cause issues when you are trying to merge OSM data after conversion as I needed the id to count up.

citizenfish commented 8 months ago

I've created a pull request

roelderickx commented 8 months ago

Hi @citizenfish, thanks for your feedback.

The --positive-id parameter is undocumented on purpose, to avoid bogous uploads to OpenStreetMap using JOSM. I still consider that a valid reason to keep the parameter undocumented.

Using positive id's JOSM will upload a file that will try to overwrite the first nodes added to OpenStreetMap around 2004-2005. If the id's are negative, JOSM considers them new to the database.

The code contains a vague warning, which was there since @pnorman added this feature: https://github.com/roelderickx/ogr2osm/blob/07440b547bd81226f30a639eda930a890507c754/ogr2osm/ogr2osm.py#L107-L108

That being said, it's not the first time someone struggles with this and their use cases are probably legit. There may have been alternative solutions to protect people from using positive id's, an interactive question for example, but I'm afraid that will break many scripts at this point.

pnorman commented 8 months ago

The goal of ogr2osm is to prepare files for import. This requires the IDs are negative in order for it to be added to OSM, and in general IDs need to be negative to be merged with OSM data.

roelderickx commented 8 months ago

Agreed.

For most other uses of ogr2osm it doesn't matter if the id's are positive or negative, they may even be strings. If you really need positive id's the undocumented option is available. I'm sure it's documented anyway using the right Google search terms.

Closing the issue and the pull request.

pnorman commented 8 months ago

If I were writing it these days I probably wouldn't hide the help, but it remains a dangerous option that only exists for data that will never be used with OSM data and is just using the OSM file format. idfile and saveid exist to create files that can be merged when the source data is in multiple files.

citizenfish commented 8 months ago

I use ogr2osm to create files for mkgmap map rendering. This means merging OSM and non-OSM data into a file sorted by id and hence my need for an id that increments beyond the highest id of the OSM data.

I believe this is a valid use case.

roelderickx commented 8 months ago

If I were writing it these days I probably wouldn't hide the help, but it remains a dangerous option that only exists for data that will never be used with OSM data and is just using the OSM file format. idfile and saveid exist to create files that can be merged when the source data is in multiple files.

Thanks for the clarification, that's how I already assumed the parameters should be used.

I use ogr2osm to create files for mkgmap map rendering. This means merging OSM and non-OSM data into a file sorted by id and hence my need for an id that increments beyond the highest id of the OSM data.

I believe this is a valid use case.

Every creative use of ogr2osm is welcomed, so yes, that's a valid use case.

Given the many questions I receive about this I may reconsider documenting the parameter. Please modify the pull request to meet these requirements:

If anything is unclear please ask.

citizenfish commented 8 months ago

Thanks I'll resubmit once done (work permitting!)

roelderickx commented 7 months ago

PR merged