Skip to content

Relaxing location data type restriction. #1084

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

Merged
merged 5 commits into from
Mar 9, 2019
Merged

Relaxing location data type restriction. #1084

merged 5 commits into from
Mar 9, 2019

Conversation

dskkato
Copy link
Contributor

@dskkato dskkato commented Mar 8, 2019

Currently, folium.Map(location) requires location data to be an instance of list or tuple. I felt it to be inconvenient since neither numpy's array nor pandas' DataFrame cannot be used as a location argument directly. In this PR, I'm trying to relax the restriction so that any iterable object with len(location)==2 can be accepted.

@ocefpaf
Copy link
Member

ocefpaf commented Mar 8, 2019

You can ignore Appveyor b/c I just merged the PR that implements it. Or rebase against latest master to get the tests going on Windows. Also, the failing notebook test seems to be a network issue. However, your change is not Python 2.7 compatible and we need a workaround there.

@dskkato
Copy link
Contributor Author

dskkato commented Mar 8, 2019

I saw Jinja2 has similar workroud. I will try and test on Python2.7.

try:
    from collections import abc
except ImportError:
    import collections as abc

https://github.com/pallets/jinja/blob/7e417c5c6ec90709f15e96a977ab59a5397a08fb/jinja2/_compat.py#L102-L105

@ocefpaf
Copy link
Member

ocefpaf commented Mar 8, 2019

I saw Jinja2 has similar workroud. I will try and test on Python2.7.

try:
    from collections import abc
except ImportError:
    import collections as abc

pallets/jinja:jinja2/_compat.py@7e417c5#L102-L105

That is probably OK until we drop support for legacy python.

@dskkato
Copy link
Contributor Author

dskkato commented Mar 8, 2019

I would like to use Collection for validation, but it's only supported 3.6 or later. So I added one helper function for the compatibility.

@@ -197,7 +209,8 @@ def test_map_build(self):

# Standard map.
self.setup()
rendered = [line.strip() for line in self.m._parent.render().splitlines() if line.strip()]
rendered = [line.strip()
Copy link
Member

Choose a reason for hiding this comment

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

Please avoid style changes in the same PR. It makes the reviewing process a little bit harder.
(Style changes are sometimes welcomed though, but preferably in a different PR.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I reverted them. My formatter accidentally modified it...

m = folium.Map(location)
assert m.location == [45.5236, -122.6750]

df = pd.DataFrame({"location": [45.5236, -122.6750]})
Copy link
Member

Choose a reason for hiding this comment

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

👍

@ocefpaf
Copy link
Member

ocefpaf commented Mar 9, 2019

This is good to go IMO but I'll let it here for a few days so @Conengmo can take a second look. Planning on merging it next week.

Thanks @dskkato!

@ocefpaf ocefpaf merged commit 2795789 into python-visualization:master Mar 9, 2019
@ocefpaf
Copy link
Member

ocefpaf commented Mar 9, 2019

Ooops. Wrong button! Sorry! Well... Now it is merged 😄

@Conengmo
Copy link
Member

Conengmo commented Mar 9, 2019

Looks good! Thanks for your contribution @dskkato and Filipe for the review.

@ocefpaf
Copy link
Member

ocefpaf commented Mar 9, 2019

And sorry again for the premature merge! I had way too many tabs open for a Saturday morning.

@Conengmo
Copy link
Member

Conengmo commented Mar 9, 2019

No problem

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.

3 participants