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

Initial stub on the road to address #20 #32

Merged
merged 12 commits into from
Jul 21, 2018
Merged

Conversation

darribas
Copy link
Collaborator

@darribas darribas commented Jul 14, 2018

Opening PR to track evolution on #20. Before merging, the following needs to be completed:

  • Method add_basemap implemented (including web and local url sources)
  • Documentation + docstrings for user-exposed functionality
  • Place.plot_map refactored to move plotting machinery into the new plotting.py module, and plot method inside the Place class.
  • Unittests

# If web source
if url[:4] == 'http':
# Zoom
if (type(zoom) == str) and (zoom.lower() == 'auto'):
Copy link
Member

Choose a reason for hiding this comment

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

isinstance(zoom, str) is better here

(and personally I would not do the lower, IMO no reason to give the user this flexibility)

Copy link
Collaborator Author

@darribas darribas Jul 21, 2018

Choose a reason for hiding this comment

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

Addressed in 5a37032, although I've left lower as I think it's a way to be gentler with people who always forget whether it's auto, Auto, AUTO, etc.

@darribas
Copy link
Collaborator Author

This fails in Python 2.7, but that relates to #36. Merging to address elsewhere and also be able to implement other issues.

@darribas darribas closed this Jul 21, 2018
@darribas darribas reopened this Jul 21, 2018
@darribas darribas merged commit 1f748d8 into master Jul 21, 2018
@darribas darribas changed the title [WIP] Initial stub on the road to address #20 Initial stub on the road to address #20 Jul 21, 2018
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

2 participants