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

Add pyppeteer_args to .render() #270

Open
wants to merge 1 commit into
base: master
from
Open

Conversation

@scythargon
Copy link

@scythargon scythargon commented Feb 21, 2019

Rebased my previous PR #166 into another branch.

Fix tests

Fix tests
Copy link
Member

@oldani oldani left a comment

A few comments hope we can get it to settle. Thank you for the time making this also the rebase trouble.

@@ -4,7 +4,7 @@ python:

# command to install dependencies
install:
- "pip install pipenv --upgrade-strategy=only-if-needed"
- "pip install git+https://github.com/pypa/pipenv.git"

This comment has been minimized.

@oldani

oldani Feb 21, 2019
Member

Pipenv is not actually giving any trouble, so I think we can skip this.

@@ -688,8 +688,7 @@ class BaseSession(requests.Session):
amongst other things.
"""

def __init__(self, mock_browser : bool = True, verify : bool = True,
browser_args : list = ['--no-sandbox']):
def __init__(self, mock_browser : bool = True, verify : bool = True):

This comment has been minimized.

@oldani

oldani Feb 21, 2019
Member

We should make pyppeteer_args be provided in the Session class rather than when calling render or arender. That way we avoid users thinking this args are per page when under the hood is per session.

def response_hook(self, response, **kwargs) -> HTMLResponse:
""" Change response enconding and replace it by a HTMLResponse. """
if not response.encoding:
response.encoding = DEFAULT_ENCODING
return HTMLResponse._from_response(response, self)

@property
async def browser(self):
async def get_browser(self, pyppeteer_args=None):

This comment has been minimized.

@oldani

oldani Feb 21, 2019
Member

If pyppeteer_args are provided on Session initialization, we can keep browser being a property.

@oldani
Copy link
Member

@oldani oldani commented Mar 31, 2019

@scythargon Any updates here? I want to merge this

@oldani
Copy link
Member

@oldani oldani commented Apr 29, 2019

@scythargon I really want to get this merged, are you still on this? I can finish this but I can figure a way to give your credits due to this.

@kennethreitz42
Copy link
Collaborator

@kennethreitz42 kennethreitz42 commented May 31, 2019

merge conflict

@14ROVI
Copy link

@14ROVI 14ROVI commented Jul 22, 2020

It would be nice if this pull request could be sorted out because its quite a breaking feature for raspberry pi / Linux devices.

It seems like i found a fix here: miyakogi/pyppeteer#250 (comment)
for people having the same problem as me

@samcdavid
Copy link

@samcdavid samcdavid commented Jul 29, 2020

@oldani @scythargon any movement on this? I know y'all are probably super busy but it would be extraordinarily helpful to get these changes merged in.

@samcdavid
Copy link

@samcdavid samcdavid commented Jul 29, 2020

If I'm reading this properly, all that needs to be done is undoing the changes to the render and arender methods. Update the BaseSession class to accept pyppeteer_args on init. Then change the get_browser method back to a property. Is that right?

samcdavid added a commit to samcdavid/requests-html that referenced this pull request Jul 29, 2020
Why
---

- Developers need the ability to set all of the pyppeteer args when
instantiating a session. This way they can set their own executable path
when running in a container, RaspberryPi, etc.

How
---

- Add pyppeteer args as a dict to the BaseSession init method.
- Use the browser args and pyppeteer args to launch pyppeteer.

Related PRs
-----------

- psf#270
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

5 participants
You can’t perform that action at this time.