Skip to content

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

Closed
wants to merge 5 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -155,5 +155,6 @@ can either use the `--source` flag when using tldr or by specifying the followin
* `TLDR_PAGES_SOURCE_LOCATION` to control where to get individual pages from
* defaults to `https://raw.githubusercontent.com/tldr-pages/tldr/master/pages`
* it can also point to local directory using `file:///path/to/directory`
* multiple sources can be specified in a ';' delimited string, with the first match being returned.
* `TLDR_DOWNLOAD_CACHE_LOCATION` to control where to pull a zip of all pages from
* defaults to `https://tldr-pages.github.io/assets/tldr.zip`
61 changes: 61 additions & 0 deletions tests/test_tldr.py
Original file line number Diff line number Diff line change
Expand Up @@ -131,3 +131,64 @@ def test_get_cache_dir_default(monkeypatch):
monkeypatch.delenv("HOME", raising=False)
monkeypatch.setattr(os.path, 'expanduser', lambda _: '/tmp/expanduser')
assert tldr.get_cache_dir() == "/tmp/expanduser/.cache/tldr"


@pytest.fixture()
def clean_cache(monkeypatch, tmp_path):
# Ensure that we have a clean cache to prevent test pollution
tmp_cache = tmp_path / "throw-away-tldr-cache"
tmp_cache.mkdir()
monkeypatch.setenv(name="XDG_CACHE_HOME", value=str(tmp_cache))


def test_get_page_handles_multiple_locations(clean_cache, tmp_path):
pages_path = tmp_path / "pages.fixture-specific-locale"
platform_path = pages_path / "fixture-specific-platform"
platform_path.mkdir(parents=True)
(platform_path / "command-under-test.md").write_text("# Content under test")

local_page_sources = [
"file:///page/source/does/not/exist",
f"file://{str(pages_path).replace('.fixture-specific-locale', '')}",
]

test_page_first_line = tldr.get_page(
command="command-under-test",
platforms=["fixture-specific-platform"],
sources=local_page_sources,
languages=["fixture-specific-locale"]
)[0]

assert b'# Content under test' in test_page_first_line


def test_default_source_configuration():
sources = tldr.get_pages_source_locations()
assert sources == [tldr.DEFAULT_SOURCE_LOCATION]


def test_source_arg_configuration():
sources = tldr.get_pages_source_locations("source_arg/")
assert sources == ["source_arg"]


def test_source_env_configuration(monkeypatch):
monkeypatch.setenv(name="TLDR_PAGES_SOURCE_LOCATION", value="env_source/")
sources = tldr.get_pages_source_locations()
assert sources == ["env_source"]


def test_source_overrides_env_configuration(monkeypatch):
monkeypatch.setenv(name="TLDR_PAGES_SOURCE_LOCATION", value="env_source/")
sources = tldr.get_pages_source_locations("source_arg")
assert sources == ["source_arg"]


# overloading this test with the slash stripping logic
def test_multiple_sources_configuration(monkeypatch):
monkeypatch.setenv(name="TLDR_PAGES_SOURCE_LOCATION", value="env_source/;env_source2")
sources = tldr.get_pages_source_locations(None)
assert sources == ["env_source", "env_source2"]

sources = tldr.get_pages_source_locations("source_arg;source_arg2/")
assert sources == ["source_arg", "source_arg2"]
75 changes: 38 additions & 37 deletions tldr.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,8 @@
__client_specification__ = "1.5"

REQUEST_HEADERS = {'User-Agent': 'tldr-python-client'}
PAGES_SOURCE_LOCATION = os.environ.get(
Copy link
Author

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

'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'
Expand All @@ -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

Expand Down Expand Up @@ -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:
Copy link
Author

Choose a reason for hiding this comment

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

Now that the global variable access is gone, remote is always has a value.

remote = PAGES_SOURCE_LOCATION

if language is None or language == 'en':
language = ''
else:
Expand Down Expand Up @@ -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
Copy link
Author

Choose a reason for hiding this comment

The 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]:
Expand All @@ -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
Copy link
Author

Choose a reason for hiding this comment

The 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

Expand Down Expand Up @@ -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
Copy link
Author

Choose a reason for hiding this comment

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

This doesn't change the default behaviour. It moves it to the get_pages_source_locations() function.

type=str,
help="Override the default page source")

Expand Down Expand Up @@ -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"
Expand Down