-
Notifications
You must be signed in to change notification settings - Fork 2.2k
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
Conversation
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. |
I saw Jinja2 has similar workroud. I will try and test on Python2.7.
|
That is probably OK until we drop support for legacy python. |
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. |
tests/test_folium.py
Outdated
@@ -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() |
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.
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.)
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 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]}) |
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.
👍
Ooops. Wrong button! Sorry! Well... Now it is merged 😄 |
Looks good! Thanks for your contribution @dskkato and Filipe for the review. |
And sorry again for the premature merge! I had way too many tabs open for a Saturday morning. |
No problem |
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 withlen(location)==2
can be accepted.