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

show_in_browser() method for folium.folium.Map #953

Conversation

Demetrio92
Copy link
Contributor

fixes #946


Adds show_in_browser() method for folium.folium.Map which should work even in a vanilla python, e.g. try:

python3 examples/OpenMapInBrowser.py

I did not write any tests for the new functionality, as I am not quite sure how to test it. Suggestions welcome.
On the other hand, some tests were failing before I added any of my code, and some packages were missing in the requirements-dev.txt. I collected all the fixes here: #952


p.s. sorry for the branch name, this should work:

git checkout 'show_in_browser()-method-for-folium.folium.Map'

examples/OpenMapInBrowser.py Outdated Show resolved Hide resolved
if sys.version_info[0] < 3:
raise NotImplementedError('Only works with python 3!')

import webbrowser

Choose a reason for hiding this comment

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

E402 module level import not at top of file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that seems to be the case for most of the other files in the repo, so I chose consistency over purity.

raise NotImplementedError('Only works with python 3!')

import webbrowser
from http.server import BaseHTTPRequestHandler, HTTPServer

Choose a reason for hiding this comment

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

E402 module level import not at top of file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that seems to be the case for most of the other files in the repo, so I chose consistency over purity.


import webbrowser
from http.server import BaseHTTPRequestHandler, HTTPServer
from textwrap import dedent

Choose a reason for hiding this comment

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

E402 module level import not at top of file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that seems to be the case for most of the other files in the repo, so I chose consistency over purity.

Copy link
Member

Choose a reason for hiding this comment

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

Please follow the liner cues here. We are cleaning up the code and new code should be compliant from the start.

folium/_server.py Show resolved Hide resolved
folium/_server.py Show resolved Hide resolved
@Demetrio92
Copy link
Contributor Author

So, err, both checks fail :(

  • Linter is right, but I just followed the convention in other files. In that sense none of the files in the repository would pass the above linter.
  • Travis is right, the feature does not work in python 2, in which case it gives a NotImplementedError with a message. Ofc, I could pass it with a print, but do we really want it? The rest seem to work flawlessly.

@ocefpaf
Copy link
Member

ocefpaf commented Sep 4, 2018

Travis is right, the feature does not work in python 2, in which case it gives a NotImplementedError with a message. Ofc, I could pass it with a print, but do we really want it? The rest seem to work flawlessly.

Raising a NotImplementedError is OK there.

HeatMap(data).add_to(folium_map)

# open in in browser
folium_map.show_in_browser()
Copy link
Member

Choose a reason for hiding this comment

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

No need for this example. This can be in the method docstring.


import sys
if sys.version_info[0] < 3:
raise NotImplementedError('Only works with python 3!')
Copy link
Member

Choose a reason for hiding this comment

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

Maybe a more verbose message like:

This feature is only available on Python 3.

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 actually realised i might have misunderstood something. Because there is an http server in python 2, so I'll check it again tomorrow.

HTTPServer.allow_reuse_address = True
httpd = HTTPServer((self.host, self.port), HTTPServerRequestHandler)

# run the temporary http server with graceful exit option
Copy link
Member

Choose a reason for hiding this comment

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

Most of these comment can be removed. I guess that they were added on SO to explain the answer. We don't need them here.

@Conengmo
Copy link
Member

Conengmo commented Sep 4, 2018

I definitely appreciate the effort @Demetrio92, but to be honest I think having an HTTP server is overkill. Currently (as far as I know) all data is embedded in the map, so you could just as easily call webbrowser.open() on a temporary HTML file, which has the added benefit of not unnecessarily blocking the program.

I learned from @ocefpaf he would like to work towards not having all data embedded in the map, which is fine. But let's implement an HTTP server when or if we get there. All code needs to be maintained, so the less the better.

@ocefpaf
Copy link
Member

ocefpaf commented Sep 4, 2018

I learned from @ocefpaf he would like to work towards not having all data embedded in the map,

That is in the long term and may never happen...

But let's implement an HTTP server when or if we get there.

Yep.

All code needs to be maintained, so the less the better.

💯

@Demetrio92
Copy link
Contributor Author

Demetrio92 commented Sep 4, 2018

@Conengmo @ocefpaf
This a bit of bummer. As far as i understood the discussion in #946 simply opening a file might randomly fail. So i've spent some time tidying up that simple http server.
I totally agree with less is better, but would you prefer to have a method that might not work as the user expects it?

In defence of the "server" code, it's base python and I really doubt there will be major compatibility breaks or maintenance required.

Please let me know if this PR generally would be accepted or not. If yes, I'll address the remaining issues, otherwise let's just close it.
I'll think of submitting another one that is based on temp-files (which i initially considered, but went for this option due to functionality).

examples/OpenMapInBrowser.py Outdated Show resolved Hide resolved
from SimpleHTTPServer import SimpleHTTPRequestHandler as BaseHTTPRequestHandler
from SocketServer import TCPServer as HTTPServer

import webbrowser

Choose a reason for hiding this comment

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

E402 module level import not at top of file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same as before this seems to be a convention in the repo

from SocketServer import TCPServer as HTTPServer

import webbrowser
from textwrap import dedent

Choose a reason for hiding this comment

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

E402 module level import not at top of file

Copy link
Contributor Author

Choose a reason for hiding this comment

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

same as before this seems to be a convention in the repo

folium/folium.py Show resolved Hide resolved
from SimpleHTTPServer import SimpleHTTPRequestHandler as BaseHTTPRequestHandler
from SocketServer import TCPServer as HTTPServer

import webbrowser

Choose a reason for hiding this comment

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

E402 module level import not at top of file

from SocketServer import TCPServer as HTTPServer

import webbrowser
from textwrap import dedent

Choose a reason for hiding this comment

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

E402 module level import not at top of file

folium/folium.py Show resolved Hide resolved
@Demetrio92
Copy link
Contributor Author

I fixed python 2.7 issue, and addressed all the feedback, except that we don't really need the feature :D

Linter complaining about E402 can keep complaining, E402 is not followed in the repository and should be excluded from stickler-ci,

@Conengmo
Copy link
Member

Close in favor of #1651

Thanks for your efforts on this, sorry it got left hanging for so long.

@Conengmo Conengmo closed this Nov 17, 2022
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.

show folium outside of ipython notebooks
4 participants