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

Italian application added. Minor bugs corrected. #147

Closed
wants to merge 12 commits into from

Conversation

guglielmo
Copy link
Contributor

I've installed mapit, using the git URL in the requirements as a django application within a pre-existing project.
I modified the .gitignore to include the django_mapit.egg-info path, automatically created.

The mapit_it application has been added, with a management task to find parents of the official ISTAT administrative subdivisions and an adapted zip code validation. It is just a prototype and other functions and tasks will follow soon.

Two minor bugs require revision, but I could not make separate pull requests (sorry)

  • 2bb188e - a string emitted to stdout as log was causing a unicode error (django 1.6.8 over python 2.7.7)
  • f7dde4c - cache value injection in a new key was cast to int; the string caused an error to the following incr instruction (cache engine is LocMem, while development)

@dracos
Copy link
Member

dracos commented Nov 28, 2014

Thank you for this, looks good :-) If you could rebase this on top of the current master, this might help with your 2bb188e (MapIt now works fully under python 3.2, so this commit hopefully is no longer necessary). And that should also fix the Travis error which was probably the Django 1.7.1 change we've worked around. Then once the tests are passing, we can look at getting it merged :)

@dracos
Copy link
Member

dracos commented Dec 5, 2014

Your branch rebased on our current master passes the tests - https://github.com/mysociety/mapit/compare/openpolis-master - and definitely doesn't need fc0880d or 2bb188e, so that's good :) Could you explain your bdf7209 - your change in f7dde4c for locmem seems to continue working in memcache here, so why did you switch it back for memcached alone?

@guglielmo
Copy link
Contributor Author

On my machine, f7dde4c didn't work with memcached, so I created the exception. Don't know if it was caused by memcache version, but it seemed a reasonable patch.

I rebased against the master, but still have errors caused by the log message (the one addressed by 2bb188e).

@dracos
Copy link
Member

dracos commented Dec 16, 2014

Very sorry about the continuing log message error – I've uncovered the problem and committed a fix in 6d1b1b8 so hopefully that really is okay now, let me know!

I'd rather not have bdf7209 unless necessary�. As I understand it, memcache uses strings as keys internally and always has done, and Django converts any key to a string before passing to memcached, so I don't see why it would require this to work, and can't find any other code on github that appears to do the same - could you provide the error you received without it?

@dracos dracos mentioned this pull request Dec 18, 2014
@dracos
Copy link
Member

dracos commented Dec 18, 2014

I have merged in #153, which includes hopefully everything from this PR that is needed. If you could check whether the updated master works for you, that'd be great :)

@guglielmo
Copy link
Contributor Author

I found out that the correct way to go, at least with python 3.2 and python 2.7,
in order to have string literals work, is to add::

from __future__ import unicode_literals 

at the beginning of the file (mapit_import.py, in this case), and leave the strings as they are.
If you don't do that, the tests may pass, but only because you do not have
names with unicode characters in the kml fixtures
;-)

Importing data (shp in my case), with names containing unicode chars raise a Unicode exception
in python 2.7.

@dracos
Copy link
Member

dracos commented Dec 22, 2014

I've put a commit altering the test similar to your commit 2ce41c6 in the test-accented-char branch to show you that it works fine on the current master without altering mapit_import.py :-) I don't think one needs to use unicode_literals, as writing either a str/unicode (py2) or bytes/str (py3) to self.stdout.write() automatically escapes correctly in both python 2 and 3 by using https://docs.djangoproject.com/en/dev/ref/utils/#django.utils.encoding.force_str which always returns the str that stdout wants, regardless of what it is given. You can indeed use unicode_literals in a test file if you want to include an accented character there, but that's all.

As some example Shapefile imports, I downloaded the "Admin 0 – Countries", the first file from http://www.naturalearthdata.com/downloads/110m-cultural-vectors/ - and ran (sorry, my db was set up for UK data types):

$ python
Python 2.7.9 (default, Dec 11 2014, 02:36:08) 
[GCC 4.2.1 Compatible Apple LLVM 5.1 (clang-503.0.40)] on darwin
Type "help", "copyright", "credits" or "license" for more information.

>>> import django
>>> django.get_version()
'1.6.7'
>>> ^D
$ python manage.py mapit_import ~/Downloads/ne/ne_110m_admin_0_countries.shp --generation_id 1 --area_type_code EUR --name_type_code M --country_code E | grep Ivo
  looking at 'Côte d'Ivoire'

screen shot 2014-12-22 at 01 57 21

I did the same with the serie_25_wgs84_geo.zip available from http://www.igmi.org/download.php :

$ python manage.py mapit_import ~/Downloads/serie_25_wgs84_geo/Serie_25_wgs84_geo.shp --generation_id 1 --area_type_code EUR --name_type_code M --country_code E --name_field TITLE --commit
[…]
  looking at 'Capo Pàssero'

And the Shapefile of Corsica from http://download.geofabrik.de/europe/france/corse.html :

$ ogr2ogr -where 'name is not null' output.shp natural.shp 
$ python manage.py mapit_import ~/Downloads/corse-latest.shp/output.shp --generation_id 1 --area_type_code EUR --name_type_code M --country_code E --commit

(The last throws up a separate issue that something (GDAL library underneath?) assumes the Shapefile is in iso-8859-1 as per spec, but it does not error when logging.)

Perhaps the difference in behaviour you are seeing is caused by some underlying GDAL version difference or something? Could you provide the shape file that doesn't work for you so we can replicate it here? Or can you try one of the examples above and see if they also don't work for you?

@dracos
Copy link
Member

dracos commented Feb 11, 2015

Closing, as it has been merged e.g. in #153 and 6d1b1b8; if you still have difficulties with the current code and can come back with answers to the questions above on shape files or memcache, please open a new ticket with details.

@dracos dracos closed this Feb 11, 2015
@TomSteinberg
Copy link

@guglielmo Are you OK on this? Just want to make sure you're all good on this project, and want to find out what's going on with you...

@guglielmo
Copy link
Contributor Author

Ooops, yes it's ok, I just overlooked at your notification.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants