Feature/issue 188/enable multiple sources #192
Conversation
We focused solely on getting a simple, but explicit get_page test that pulls content from a local repository to enable the next refactoring commits. Signed-off-by: David Laing <[email protected]>
Additionally, updated get_page method interface to receive non-optional source (which was orignally called remote) Signed-off-by: David Laing <[email protected]>
This is in preparation for the follow new mutli-source functionality Signed-off-by: David Laing <[email protected]>
Sources are treated in prioritised order - first match will be used Signed-off-by: David Laing <[email protected]>
| @@ -21,10 +21,8 @@ | |||
| __client_specification__ = "1.5" | |||
|
|
|||
| REQUEST_HEADERS = {'User-Agent': 'tldr-python-client'} | |||
| PAGES_SOURCE_LOCATION = os.environ.get( | |||
There was a problem hiding this comment.
Removed as part of the refactor of the Global variable to the get_pages_source_locations() function
| @@ -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.
Now that the global variable access is gone, remote is always has a value.
| remote: Optional[str] = None, | ||
| sources: List[str], |
There was a problem hiding this comment.
We felt the domain language update added some clarity here.
| 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 |
There was a problem hiding this comment.
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)
| default=PAGES_SOURCE_LOCATION, | ||
| default=None, |
There was a problem hiding this comment.
This doesn't change the default behaviour. It moves it to the get_pages_source_locations() function.
|
Here's a demo if it in action: https://www.loom.com/share/08ed2a0053cd4c019412e33aa47e2bd8 |
|
@owenvoke would you be able to take a look at this? Cheers |
|
Sorry for the delay on looking at this, will try to get to this sometime this week. |
|
@MasterOdin nudge nudge 😄 |
|
Closing since I'm in favour of using https://github.com/charmbracelet/mods for unix program explanations. I'll work withing the constraints of local scripts/wikis/issue templates for sharing company specific stuff (which is what this PR was meant to enable). |
Hi @MasterOdin , @mrdavidlaing and I would like to submit this PR which relates to Issue #188 and potentially #147 and #146.
Hope you appreciate the additional tests!
If you like what you see, and would like more, we'll follow up with another PR. We have some ideas around some high-level functional testing of
mainbut didn't want to add too much noise to this PR. We also believe we can useitertools.productto simplify the nesting of the loops.We'll comment inline in this PR to some of the code snippets that might appear confusing at first glance.