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

Remove OpenStreetMap integration in analysis lib + app #5354

Merged
merged 1 commit into from Oct 13, 2017

Conversation

wonder-sk
Copy link
Member

Why:

  • unmaintained and barely working anymore
  • clunky GUI
  • not supporting .pbf
  • there are other python plugins doing the same thing better
  • nobody is using it anymore

For reference: https://lists.osgeo.org/pipermail/qgis-developer/2017-October/050103.html

@Gustry
Copy link
Contributor

Gustry commented Oct 12, 2017

Can we keep the qgsosmdownload.h? I was going to use it while I'm porting QuickOSM to QGIS 3.
Or do you think I should implement a new one?

not supporting .pbf

This is not a problem. Overpass API is not supporting PBF as an output so we can't avoid the XML file. But I agree, we should use the OSM driver in OGR (supporting XML and PBF)

@wonder-sk
Copy link
Member Author

Can we keep the qgsosmdownload.h? I was going to use it while I'm porting QuickOSM to QGIS 3.
Or do you think I should implement a new one?

It is quite a small and simple file, so it would be a better if you port it to Python for your use case... I don't think we should keep it around just for that...

@DelazJ
Copy link
Contributor

DelazJ commented Oct 12, 2017

@wonder-sk could you add a [needs-docs] tag, please? This feature is documented in the user manual and is the base tool to download data in the Training manual. Hence docs would need some update...

@Gustry
Copy link
Contributor

Gustry commented Oct 12, 2017

no worries, I was just lazzy ;-)

It seems PBF is coming on the overpass API now: http://gis.19327.n8.nabble.com/PBF-output-prototype-available-for-testing-td5884534.html good news !

So yes, maybe better to use the OGR driver now with PBF

@wonder-sk
Copy link
Member Author

@DelazJ that tag needs to go to the commit message or is pull request title also a good place?

@DelazJ
Copy link
Contributor

DelazJ commented Oct 12, 2017

@wonder-sk From what i've experienced, renaming the PR isn't enough (in my case this renaming didn't generate the expected issue report in Docs)
FYI the hook is at https://github.com/qgis/QGIS-Sysadmin/blob/master/webhooks/github_feature_tracker.cgi#L420 if you want to be sure (I do not understand all the code)

@Gustry
Copy link
Contributor

Gustry commented Oct 12, 2017

It might depend if you rebase or merge your PR? Updating the title of the PR might be useless if you do a rebase on github. My 2 cents

- unmaintained and barely working anymore
- clunky GUI
- not supporting .pbf
- there are other python plugins doing the same thing better
- nobody is using it anymore

https://lists.osgeo.org/pipermail/qgis-developer/2017-October/050103.html
@wonder-sk
Copy link
Member Author

@DelazJ cool - I have updated the commit message to include [needs-docs]

@nyalldawson
Copy link
Collaborator

@Gustry

It is quite a small and simple file, so it would be a better if you port it to Python for your use case... I don't think we should keep it around just for that...

Also be aware that the if you're porting that class to python, you might want to take a look at QgsFileDownloader and take inspiration from it instead. It has correct handling for redirects and authentication handling, which the OSM downloader doesn't have.

Copy link
Collaborator

@nyalldawson nyalldawson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's kinda sad to see this support go, but I agree with the rationale.

@wonder-sk wonder-sk merged commit ca0aa72 into qgis:master Oct 13, 2017
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

Successfully merging this pull request may close these issues.

None yet

4 participants