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

[low urgency] [docs] add note about cache timeout argument #159

Open
floer32 opened this issue Mar 5, 2019 · 2 comments
Open

[low urgency] [docs] add note about cache timeout argument #159

floer32 opened this issue Mar 5, 2019 · 2 comments
Labels
docs good first issue help wanted contributions welcome, submit a PR low priority: has trivial workaround see issue #158 [one fell swoop] fixed when we fix https://github.com/john-kurkowski/tldextract/issues/158 - temporary label.

Comments

@floer32
Copy link
Collaborator

floer32 commented Mar 5, 2019

from PR #154 (unmerged), changing that to a docs ticket

Would be good to add a note to the README though since it does not point out this environment variable being available. [ TLDEXTRACT_CACHE_TIMEOUT is the current name]

it's undocumented currently, that you can set a timeout on the PSL-fetch request timeout.

could be added to README. but want the variable renamed...

@floer32
Copy link
Collaborator Author

floer32 commented Mar 5, 2019

variable rename notes

one wart: this variable has a name that could be misinterpreted ... it's not a 'cache timeout' in the sense of 'cache expiration', it's the timeout on the request which fetches the PSL -- which is then cached. (i think that's my fault! from a change a few years ago? 😅)

i don't think we should remove this variable name, at least without the normal DeprecationWarning process and then a major version upgrade eventually. however that seems like a yak shave.

could change it to os.environ.get('TLDEXTRACT_DEFAULT_FETCH_TIMEOUT, os.environ.get('TLDEXTRACT_CACHE_TIMEOUT')) so it uses a new better name, and document just that new name; with the old one silently supported as a legacy thing.

@floer32
Copy link
Collaborator Author

floer32 commented Mar 5, 2019

is this another "don't do anything, just do #158?"

timeout is something that can be controlled by requests.Session customization i.e. #158. so should we dismiss this ticket?

well, timeout is such a common need, and so simple to specify from an environment variable, that I get why we might retain the environment variable. with the rename noted above.

but it does mean that it's a very low priority issue.

PRs welcome anyways, to rename the variable, and add a little note to the docs.

@floer32 floer32 added low priority: has trivial workaround see issue #158 [one fell swoop] fixed when we fix https://github.com/john-kurkowski/tldextract/issues/158 - temporary label. docs help wanted contributions welcome, submit a PR good first issue labels Mar 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
docs good first issue help wanted contributions welcome, submit a PR low priority: has trivial workaround see issue #158 [one fell swoop] fixed when we fix https://github.com/john-kurkowski/tldextract/issues/158 - temporary label.
Projects
None yet
Development

No branches or pull requests

1 participant