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.

Sign up for GitHub

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

Jump to bottom

ProxyAdded proxy option + proxy sanity checks #277

Open
wants to merge 6 commits into
base: master
Choose a base branch
from
Open

ProxyAdded proxy option + proxy sanity checks #277

wants to merge 6 commits into from

Conversation

holzkohlengrill
Copy link

Added proxy option as a command line argument.

Added checks to make sure the proxy URL is valid.

lorenzodifuccia
lorenzodifuccia requested changes Aug 24, 2021
View reviewed changes
Copy link
Owner

@lorenzodifuccia lorenzodifuccia left a comment

Choose a reason for hiding this comment

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

Hi @holzkohlengrill, thank you for the PR.
Could you please see the comment on the changed file?

safaribooks.py Outdated
if USE_PROXY: # DEBUG
self.session.proxies = PROXIES
self.session.verify = False
if args_parsed.proxies:
Copy link
Owner

Choose a reason for hiding this comment

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

Use self.args instead of global args_parsed

Copy link
Author

Choose a reason for hiding this comment

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

@lorenzodifuccia please check. I removed now completely the global arguments and packed it into some functions to avoid errors like this and make it more readable.

@holzkohlengrill
Copy link
Author

@lorenzodifuccia anything open or can we merge?

lorenzodifuccia
lorenzodifuccia requested changes Oct 7, 2021
View reviewed changes
safaribooks.py Outdated Show resolved Hide resolved
safaribooks.py Outdated Show resolved Hide resolved
@lorenzodifuccia
Copy link
Owner

Hi @holzkohlengrill, I thought I sent the request change, sorry 🤦🏻‍♂️

@lorenzodifuccia
Copy link
Owner

The Proxy args and code it's ok.
But, why you changed also the way the program handle the arguments?
Could you rebase the code from the master branch and add only the proxy's related code?

@holzkohlengrill
Copy link
Author

I changed it because it was very messy and error prone due to a mix of global and local arguments, etc.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lorenzodifuccia lorenzodifuccia Awaiting requested review from lorenzodifuccia

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@holzkohlengrill @lorenzodifuccia