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 option to update TLDs list over proxy #150

Closed
wants to merge 2 commits into from

Conversation

czechnology
Copy link

I am in a scenario where the Internet can only be accessed over a proxy, yet that proxy is not meant to be in env. vars.
This PR adds option to update the TLDs over a proxy, without unnecessarily cluttering the main class initializer with proxies.

@floer32
Copy link
Collaborator

floer32 commented Mar 4, 2019

First reaction was - hmm I wish we did not have to cascade this argument through a number of places. But I do agree it's ideal not to have it in the constructor. Just wondering if at this point, should it support passing in a Session instance instead of this one argument?

@john-kurkowski thoughts?

@john-kurkowski
Copy link
Owner

I agree @hangtwenty. Let's not maintain our own requests shim language, always playing catchup. Let the caller optionally decide how we should make requests, with whatever requests options they want.

Copy link
Collaborator

@floer32 floer32 left a comment

Choose a reason for hiding this comment

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

@czechnology Support the overall idea but if we are opening it up to customization let's really open it up. Could you change it to support customization by passing Session? this will allow the proxy use case (but probably won't even mention the proxy argument specifically in the code now, as it's a requests detail, not us).

ignore snippet I just deleted, I meant to delete that before posting (realized that part was wrong)

@floer32 floer32 added the see issue #158 [one fell swoop] fixed when we fix https://github.com/john-kurkowski/tldextract/issues/158 - temporary label. label Mar 5, 2019
@john-kurkowski
Copy link
Owner

Closing due to inactivity. Concentrate on #158.

@john-kurkowski john-kurkowski mentioned this pull request Aug 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hmm 🤔 see issue #158 [one fell swoop] fixed when we fix https://github.com/john-kurkowski/tldextract/issues/158 - temporary label.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants