Skip to content

Conversation

@ocefpaf
Copy link
Member

Closes#891

I chose firefox b/c it is easier to test on Travis-CI and we found some rendering issues with chrome/chromium in the past.

PS: @Conengmo the _png() is not a first class citizen yet. Not sure what else we could do to make it more robust. Maybe a driver selection option?

@ocefpafocefpaf requested a review from ConengmoNovember 7, 2018 17:26
import time
import warnings

from branca.colormap import StepColormap
Copy link
MemberAuthor

Choose a reason for hiding this comment

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

This is a bad rebase :-/ Let me fix those.

@ocefpaf
Copy link
MemberAuthor

@Conengmo rebased and ready for a review. This one is not too important b/c we don't really advertise this functionality yet.

@Conengmo
Copy link
Member

Conengmo commented Nov 15, 2018

Looks good, and the tests work fine on Travis. Only problem is that the temporary file doesn't work on Windows. We had this problem before:
https://docs.python.org/3.6/library/tempfile.html#tempfile.NamedTemporaryFile

I'll see if I can find a solution, otherwise we'll merge it as is.

@ConengmoConengmo added in discussion This PR or issue is being discussed and removed waiting for review PR is waiting to be reviewed labels Nov 16, 2018
@Conengmo
Copy link
Member

I can get the tempfile stuff to work if instead of using NamedTemporaryFile we use the underlying mkstemp function. We have to close and delete the file ourselves, but it's not that hard.

@contextmanager def _tmp_html(data): """Yields the path of a temporary HTML file containing data.""" filepath = '' try: f_int, filepath = tempfile.mkstemp(suffix='.html', prefix='folium_') os.write(f_int, data.encode('utf8')) os.close(f_int) yield filepath finally: if os.path.isfile(filepath): os.remove(filepath) 

Note that this returns the file path, not a file descriptor. What do you think @ocefpaf?

@ocefpaf
Copy link
MemberAuthor

ocefpaf commented Nov 16, 2018

Note that this returns the file path, not a file descriptor. What do you think @ocefpaf?

I'm fine with that solution. (Actually I used something similar in the past when writing netCDF files, my bad for ignoring Windows here.)

@ocefpaf
Copy link
MemberAuthor

@Conengmo I'll take a look at the failures later but it seems that only Python 2.7 is affected and, b/c this feature is not really ready for prime time, I'm OK making it Python 3 only. (We already agreed that the next release is the last one for Python 2.7 anyway.)

def m_png():
yield folium.Map(png_enabled=True)

def test__repr_html_is_str(m):

Choose a reason for hiding this comment

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

E302 expected 2 blank lines, found 1

@ocefpaf
Copy link
MemberAuthor

@Conengmo I'll take a look at the failures later but it seems that only Python 2.7 is affected and, b/c this feature is not really ready for prime time, I'm OK making it Python 3 only. (We already agreed that the next release is the last one for Python 2.7 anyway.)

Spoke too soon. All the failures were related to new pytest version. This one is good to go now.

@ocefpaf
Copy link
MemberAuthor

ocefpaf commented Nov 20, 2018

Merging this to b/c of the Travis-CI fixes in the last commit are needed for a new release.

@ocefpafocefpaf merged commit 8adfb77 into python-visualization:masterNov 20, 2018
@ocefpafocefpaf deleted the firefox_headless branch November 20, 2018 17:53
@ConengmoConengmo removed the in discussion This PR or issue is being discussed label Nov 21, 2018
Sign up for freeto 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

@ocefpaf@Conengmo@stickler-ci