Skip to content

Use firefox headless instead of phantomjs #1015

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 7 commits into from
Nov 20, 2018

Conversation

ocefpaf
Copy link
Member

@ocefpaf ocefpaf commented Nov 7, 2018

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?

@ocefpaf ocefpaf requested a review from Conengmo November 7, 2018 17:26
folium/folium.py Outdated
import time
import warnings

from branca.colormap import StepColormap
Copy link
Member Author

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
Member Author

ocefpaf commented Nov 15, 2018

@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.

@Conengmo Conengmo 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
Member Author

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
Member Author

ocefpaf commented Nov 16, 2018

@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
Member Author

ocefpaf commented Nov 16, 2018

@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
Member Author

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.

@ocefpaf ocefpaf merged commit 8adfb77 into python-visualization:master Nov 20, 2018
@ocefpaf ocefpaf deleted the firefox_headless branch November 20, 2018 17:53
@Conengmo Conengmo removed the in discussion This PR or issue is being discussed label Nov 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.

3 participants