Optional command-line keyword arguments #192
Conversation
There was a problem hiding this comment.
I think this is a breaking change, since previous commandline invocations will no longer work. Maybe we can make just --repo-dir into a named parameter? That way, if the current invocation is gitpuller <repo> <branch> it will continue to work. Minimal breaking :D
We can also use that opportunity to call it --target-dir or something like that instead?
nbgitpuller/pull.py
Outdated
| @@ -303,14 +303,14 @@ def main(): | |||
|
|
|||
| parser = argparse.ArgumentParser(description='Synchronizes a github repository with a local repository.') | |||
| parser.add_argument('git_url', help='Url of the repo to sync') | |||
| parser.add_argument('branch_name', default=None, help='Branch of repo to sync', nargs='?') | |||
| parser.add_argument('repo_dir', default='.', help='Path to clone repo under', nargs='?') | |||
| parser.add_argument('--branch_name', default=None, required=False, help='Branch of repo to sync', nargs='?') | |||
There was a problem hiding this comment.
| parser.add_argument('--branch_name', default=None, required=False, help='Branch of repo to sync', nargs='?') | |
| parser.add_argument('--branch-name', default=None, required=False, help='Branch of repo to sync') |
There was a problem hiding this comment.
I think flags starting with -- by default are not required and set default to None, so maybe those are not required?
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
+1 for consistently using python_snake_case and letting the --python-snake-case flag be assumed as a result when using add_argument, it is what I've seen across the JupyterHub org and become used to.
There was a problem hiding this comment.
ah, ok! I wasn't aware of this behavior! I've resolved this suggestion
There was a problem hiding this comment.
Conclusion: i got things wrong, ignore me
Without -- added, it will be interpreted as a positional argument it seems, so adding -- is relevant, but if its added, then we should use - as a separator instead of _ like @yuvipanda suggested as its whats common across the JupyterHub org.
There was a problem hiding this comment.
OK got it so "-" as separator but we are worried about the breaking change so we will only do this with repo-dir but call it target-dir. Good deal
nbgitpuller/pull.py
Outdated
| parser.add_argument('branch_name', default=None, help='Branch of repo to sync', nargs='?') | ||
| parser.add_argument('repo_dir', default='.', help='Path to clone repo under', nargs='?') | ||
| parser.add_argument('--branch_name', default=None, required=False, help='Branch of repo to sync', nargs='?') | ||
| parser.add_argument('--repo_dir', default='.', required=False, help='Path to clone repo under', nargs='?') |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
| parser.add_argument('--repo_dir', default='.', required=False, help='Path to clone repo under', nargs='?') | |
| parser.add_argument('repo_dir', default='.', required=False, help='Path to clone repo under', nargs='?') |
tests/test_gitpuller.py
Outdated
| cmd = ['python3', 'pull.py', remote_path, branch, pusher_path] | ||
| cmd = ['python3', 'pull.py', remote_path] | ||
| if branch is not None: | ||
| cmd += ['--branch_name', branch] |
There was a problem hiding this comment.
| cmd += ['--branch_name', branch] | |
| cmd += ['--branch-name', branch] |
There was a problem hiding this comment.
I pushed another commit into the PR:
- branch_name is optional but remains positional
- repo_dir becomes --target-dir -- I did not change repo_dir to target_dir in the rest of the code base; I could but repo_dir makes sense to the developer in the context but I agree could make less sense or no sense to a user invoking from the command-line.
Thanks for the feedback.
tests/test_gitpuller.py
Outdated
| if branch is not None: | ||
| cmd += ['--branch_name', branch] | ||
| if pusher_path is not None: | ||
| cmd += ['--repo_dir', pusher_path] |
There was a problem hiding this comment.
| cmd += ['--repo_dir', pusher_path] | |
| cmd += ['--repo-dir', pusher_path] |
The optional third command-line argument, repo_dir, is now a keyword argument and the name is changed to target-dir; this allows us to avoid having to pass an empty branch_name as the second argument if you want to pass in the repo_dir(now target-dir) as the third argument. In the previous configuration, we used positional arguments: gitpuller git_url branch_name repo_dir Now, we use a keyword argument for target-dir: gitpuller git_url [branch_name] --target-dir [TARGET_DIR]
|
Awesome! @consideRatio how do we document this breaking change? CHANGELOG? |
|
1.0.0 release and changelog? :) |
|
@consideRatio Agree on landing this for 1.0. But I think we should get #177 fixed before 1.0. |
|
I can document in the changelog but what version are we bumping? 1.0 and wait on this PR for #177 or 0.11 and merge this piece? |
|
I would love to see a fix to this master vs main in nbgitpuller |
The optional command-line arguments(repo-dir and branch_name) are now keyword arguments; this allows us to avoid having to pass an empty branch_name(e.g. "") as the second argument if you want to pass in the repo_dir as the third argument. In the previous configuration, we used positional arguments(gitpuller git_url branch_name repo_dir) this change uses keyword arguments: gitpuller git_url --branch_name [BRANCH_NAME] --repo_dir [REPO_DIR]
resolves #187