search

ckan / ckanext-spatial

Geospatial extension for CKAN
http://docs.ckan.org/projects/ckanext-spatial
126 stars 193 forks source link

fix crash caused by new GeoAlchemy2 version #215

Closed anotheredward closed 5 years ago

anotheredward commented 5 years ago

This patch fixes an issue where new versions of GeoAlchemy2 caused crashes.

We've locked the dependency to a known, working version.

anotheredward commented 5 years ago

I also noted that doc-requirements.txt has a reference to GeoAlchemy>=0.6 and no reference to GeoAlchemy2, is this correct?

kmbn commented 5 years ago

The checks fail when building against CKAN master because the functional tests rely on Pylons rather than Flask routing. I submitted a PR to @anotheredward fix the functional tests; if that's merged, then all the checks for this PR will pass and it can be merged.

kmbn commented 5 years ago

@amercader Coming back to @anotheredward's question about doc-requirements.txt, do we need GeoAlchemy, OWSLib, lxml and pyparsing there at all?

amercader commented 5 years ago

@anotheredward thanks for this

I might be remembering this all wrong but Read The Docs needs the same requirements as the extension to build (In CKAN core we basically install all requirements). If someone want to check, this should be easy to test removing the requirements and see if the docs build on RTD. @anotheredward Geoalchemy2 per se is not required in there because the extension works with GeoAlchemy only also.

@kmbn can you submit your tests fix PR here? It looks great and should be on master

kmbn commented 5 years ago

@amercader Yes, I'll submit it here (somehow I thought it wasn't possible to merge a PR if any of the builds failed, so I thought I'd try to get them working on this one).

kmbn commented 5 years ago

The docks build on RTD without GeoAlchemy, etc.: https://ckanext-spatial-test.readthedocs.io/en/latest/index.html (That's a build with nothing but Sphinx and the Sphinx RTD theme in the requirements.)

anotheredward commented 5 years ago

Thanks for getting this merged @kmbn @amercader !