Added source parameter #42
Conversation
|
Please note that when applying this PR, PR #44 will not be able to be merged automatically. To solve confilcts look at iliakonnov@2e6f08c |
|
LGTM. Pinging @HELLOKITTY111 (who opened #12) and @agnivade for review. |
agnivade
left a comment
agnivade
left a comment
There was a problem hiding this comment.
Hey @iliakonnov - Please have a look at my comments.
Did you test your changes ?
| output(get_page(command, options.os)) | ||
| output(get_page(command, options.source, options.os)) | ||
| else: | ||
| output(get_page(command)) |
There was a problem hiding this comment.
options.source param missing here
tldr.py
Outdated
|
|
||
|
|
||
| def download_and_store_page_for_platform(command, platform): | ||
| def download_and_store_page_for_platform(command, platform, remote): |
There was a problem hiding this comment.
There are references in the code to this function. Please pass the remote variable there too.
tldr.py
Outdated
|
|
||
|
|
||
| def get_page_for_platform(command, platform): | ||
| def get_page_for_platform(command, platform, remote): |
There was a problem hiding this comment.
You have to pass the remote here too inside the get_page call from where this function is called.
|
I think now it's OK. 6bad1af |
|
|
||
|
|
||
| def get_page_for_platform(command, platform): | ||
| def get_page_url(platform, command, remote=None): |
There was a problem hiding this comment.
Why not make remote=DEFAULT_REMOTE itself as the default param ? Then you don't need to make the check.
There was a problem hiding this comment.
Because using mutable types as default value is not good. https://docs.quantifiedcode.com/python-anti-patterns/correctness/mutable_default_value_as_argument.html
| def get_page_url(platform, command, remote=None): | ||
| if remote is None: | ||
| remote = DEFAULT_REMOTE | ||
| return remote + "/" + platform + "/" + quote(command) + ".md" |
There was a problem hiding this comment.
I'm just copied code from previous functions. Also, os.path.join is for file path, not url.
tldr.py
Outdated
| help="Override the operating system [linux, osx, sunos]") | ||
|
|
||
| parser.add_argument('-s', '--source', | ||
| default="http://raw.github.com/tldr-pages/tldr/master/pages", |
There was a problem hiding this comment.
Use the DEFAULT_REMOTE variable here ?
|
Updated as requested: 4a53815 |
|
Please rebase from latest master and resolve the merge conflicts. |
|
I'm not really with git. Is it OK? |
|
@iliakonnov The webui doesn't show any merge conflicts, so you should be good there. And they are able to do a squash merge to reduce your total commits from 5 to 1. IMO there is no further action needed on your end, pending any other code issues brought up. |
4 participants
Closes #12