1. Mikhail Korobov
  2. django-easy-maps

Pull requests

#2 Declined
Repository
everett_toews
Branch
latlong
Repository
kmike
Branch
default

Added feature to allow latitude and longitude instead of an addresss. Backwards compatible so no changes should need to be made to existing code.

Author
  1. everett_toews
Reviewers
Description

An implementation for issue #8. My first pull request on BitBucket so I hope I did it right. Let me know what you think.

Comments (4)

  1. Mikhail Korobov repo owner

    Thanks for the code! Unfortunately there are some issues with this pull request:

    • There is an ambiguity in syntax now: {% easy_map address width height zoom %} may be parsed as {% easy_map latitude longitude width height %} . It was possible to have both width-height and zoom optional because there was a way to distinguish them: 3 positional arguments -> address,width,height; 2 positional arguments -> address,zoom.
    • Address.address is NOT NULL is database (and blank is also False) but it may become empty if address is created using lat/lon. This may cause issues for admin; we would probably want to add default='' blank=True to address field if it can be empty.
    • There is an unindexed lookup for latitude/longitude in get_or_create; db_index=True should be added to these fields.
  2. everett_toews author

    Actually the way I was using the template tag was to quote latitude and longitude (e.g. "11.1234 22.1234") just as you would quote an address so the argument was always treated as one token. That's why I didn't need to touch your parsing code in the method easy_map and why I could use location.split() in the get_address method. But I didn't make this very clear in my updates to the syntax docs and errors. I'll correct that.

    Of course your points for 2 and 3 are easy enough to do too.

    However, I was trying a few different things and I think I discovered a bug in the original code. In a *freshly* pip installed version of django-easy-maps I included the following in a template.

    {% easy_map "Vancouver, BC, Canada" using 'easy_maps/map.html' %} {% easy_map "Edmonton, AB, Canada" 300 400 using 'easy_maps/map.html' %} {% easy_map "Regina, SK, Canada" 20 using 'easy_maps/map.html' %} {% easy_map "Winnipeg, MB, Canada" 300 400 20 using 'easy_maps/map.html' %}

    But an error is raised when parsing the "Regina, SK, Canada" tag at line 31 of easy_maps_tags.py. This is because when you only use the zoom option (i.e. 2 positional arguments -> address,zoom) then len(params) == 3 and that case raises the error in the original code.

    Can you confirm this bug?

    If it's a legitimate bug, I can fix it along with my other changes to this pull request or I can create another issue and fix it separately.

  3. Mikhail Korobov repo owner

    These are valid points. Please give me some time to write unit tests (there are some drafts but nothing real) and then I'll be back. I won't be able to do this until next week so if you have time help is appreciated.

  4. everett_toews author

    django-easy-maps is a great project but I don't think it quite fits my now and future requirements. Instead of trying to shoehorn in my changes to django-easy-maps I've decided to use another mapping project. I'll stop this pull request as there's no sense in needlessly complicating your code.

    Thanks for your time.