-
-
Notifications
You must be signed in to change notification settings - Fork 102
Feature/issue 188/enable multiple sources #192
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
Changes from all commits
6605170
6c5da6c
1992a02
512b7de
b772010
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -21,10 +21,8 @@ | |
__client_specification__ = "1.5" | ||
|
||
REQUEST_HEADERS = {'User-Agent': 'tldr-python-client'} | ||
PAGES_SOURCE_LOCATION = os.environ.get( | ||
'TLDR_PAGES_SOURCE_LOCATION', | ||
'https://raw.githubusercontent.com/tldr-pages/tldr/master/pages' | ||
).rstrip('/') | ||
DEFAULT_SOURCE_LOCATION = 'https://raw.githubusercontent.com/tldr-pages/tldr/master/pages' | ||
|
||
DOWNLOAD_CACHE_LOCATION = os.environ.get( | ||
'TLDR_DOWNLOAD_CACHE_LOCATION', | ||
'https://tldr-pages.github.io/assets/tldr.zip' | ||
|
@@ -48,6 +46,13 @@ | |
} | ||
|
||
|
||
def get_pages_source_locations(source_arg=None): | ||
source_locations = source_arg \ | ||
or os.environ.get('TLDR_PAGES_SOURCE_LOCATION') \ | ||
or DEFAULT_SOURCE_LOCATION | ||
return [location.rstrip('/') for location in source_locations.split(';')] | ||
|
||
|
||
class CacheNotExist(Exception): | ||
pass | ||
|
||
|
@@ -129,9 +134,6 @@ def have_recent_cache(command: str, platform: str, language: str) -> bool: | |
|
||
|
||
def get_page_url(command: str, platform: str, remote: str, language: str) -> str: | ||
if remote is None: | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Now that the global variable access is gone, |
||
remote = PAGES_SOURCE_LOCATION | ||
|
||
if language is None or language == 'en': | ||
language = '' | ||
else: | ||
|
@@ -231,7 +233,7 @@ def get_language_list() -> List[str]: | |
|
||
def get_page( | ||
command: str, | ||
remote: Optional[str] = None, | ||
sources: List[str], | ||
Comment on lines
-234
to
+236
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We felt the domain language update added some clarity here. |
||
platforms: Optional[List[str]] = None, | ||
languages: Optional[List[str]] = None | ||
) -> Union[str, bool]: | ||
|
@@ -241,32 +243,34 @@ def get_page( | |
languages = get_language_list() | ||
# only use cache | ||
if USE_CACHE: | ||
for remote in sources: | ||
for platform in platforms: | ||
for language in languages: | ||
if platform is None: | ||
continue | ||
try: | ||
return get_page_for_platform( | ||
command, | ||
platform, | ||
remote, | ||
language, | ||
only_use_cache=True, | ||
) | ||
except CacheNotExist: | ||
continue | ||
for remote in sources: | ||
for platform in platforms: | ||
for language in languages: | ||
if platform is None: | ||
continue | ||
try: | ||
return get_page_for_platform( | ||
command, | ||
platform, | ||
remote, | ||
language, | ||
only_use_cache=True, | ||
) | ||
except CacheNotExist: | ||
continue | ||
for platform in platforms: | ||
for language in languages: | ||
if platform is None: | ||
continue | ||
try: | ||
return get_page_for_platform(command, platform, remote, language) | ||
except HTTPError as err: | ||
if err.code != 404: | ||
raise | ||
except URLError: | ||
if not PAGES_SOURCE_LOCATION.startswith('file://'): | ||
raise | ||
return get_page_for_platform(command, platform, remote, language) | ||
except HTTPError as err: | ||
if err.code != 404: | ||
raise | ||
except URLError: | ||
if not remote.startswith('file://'): | ||
raise | ||
Comment on lines
+246
to
+273
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Unfortunately this looks scarier than the actual change. The main addition here is the additional loop due to multiple sources. Removing the global variable reference also subtracts from the diff clarity (line 268 of the removal, 272 of the addition) |
||
|
||
return False | ||
|
||
|
@@ -444,7 +448,7 @@ def create_parser() -> ArgumentParser: | |
help="List all available commands for operating system") | ||
|
||
parser.add_argument('-s', '--source', | ||
default=PAGES_SOURCE_LOCATION, | ||
default=None, | ||
Comment on lines
-447
to
+451
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This doesn't change the default behaviour. It moves it to the |
||
type=str, | ||
help="Override the default page source") | ||
|
||
|
@@ -542,20 +546,17 @@ def main() -> None: | |
page = i | ||
|
||
if page: | ||
result = get_page(page, None, options.platform, options.language) | ||
sources = get_pages_source_locations(options.source) | ||
result = get_page(page, sources, options.platform, options.language) | ||
output(result, plain=options.markdown) | ||
else: | ||
print("No results found") | ||
|
||
else: | ||
try: | ||
command = '-'.join(options.command).lower() | ||
result = get_page( | ||
command, | ||
options.source, | ||
options.platform, | ||
options.language | ||
) | ||
sources = get_pages_source_locations(options.source) | ||
result = get_page(command, sources, options.platform, options.language) | ||
if not result: | ||
sys.exit(( | ||
"`{cmd}` documentation is not available.\n" | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed as part of the refactor of the Global variable to the
get_pages_source_locations()
function