-
Notifications
You must be signed in to change notification settings - Fork 194
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
Supports Python3 #249
Supports Python3 #249
Conversation
…m ckan/test-core-circle-ci.ini
|
I have just tested the branch https://github.com/smellman/ckanext-spatial/tree/dev-py3 in ckan-2.9 python3 ubuntu20.04 and it worked for me (unlike the current ckan/ckanext-spatial which did not) |
Any idea when this could get merged? My organization is trying to move to CKAN 2.9 and python 3, and we rely heavily on this extension (harvesting ISO 19139). Thanks for any info! |
I have also run some preliminary tests with this WIP branch on my CKAN 2.9 setup and it seemed to work. Thanks for a great work @smellman |
ckanext/spatial/commands/spatial.py
Outdated
min_args = 0 | ||
|
||
def command(self): | ||
self._load_config() | ||
print '' | ||
print('') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we need this line ?!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This dates back >10 years! I guess load_config()
printed some logging, and this new-line made it neat. I don't know if it's still needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will delete this line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
+1 to be merged @ckan/core |
@@ -33,6 +34,11 @@ | |||
from ckanext.spatial.model import ISODocument | |||
from ckanext.spatial.interfaces import ISpatialHarvester | |||
|
|||
if p.toolkit.check_ckan_version("2.9"): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How about min_version="2.9"
, to be compatible with 3.0 when it comes out?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This improvement could go into a second PR - there's no point delaying merging this PR.
I'll do a final review this week, thanks @smellman and @smotornyuk for your work on this! |
I've merged this branch plus a bunch of improvements and fixes to make it more future-proof (you can check commits 17c7ffd to 90ee774) and tagged it Huge thanks to @smellman for finishing the great work that @smotornyuk started and taking this over the line. |
resource tag seems deprecated so I remove resource tag and use asset tag.This PR doesn't support Python2 for CI and I test with only python3 environment.