import_srpm: make most cli parameters optional #749
Conversation
the repository path defaults to . the local branch is used if not provided on the command line Signed-off-by: Gaëtan Lehmann <[email protected]>
| parser.add_argument('repository', help='local path to the repository') | ||
| parser.add_argument('parent_branch', help='git parent branch from which to branch') | ||
| parser.add_argument('branch', help='destination branch') | ||
| parser.add_argument('repository', default='./', nargs='?', help='local path to the repository') | ||
| parser.add_argument('parent_branch', nargs='?', help='git parent branch from which to branch') | ||
| parser.add_argument('branch', nargs='?', help='destination branch') |
There was a problem hiding this comment.
Maybe we could take this opportunity to use options for those? IIUC, specifying branch cannot be done without passing the first 2 ones (and parent_branch is indeed needed much less often than branch)
There was a problem hiding this comment.
I'm not using those parameters, so I can't really say.
@stormi ?
| parser.add_argument('repository', help='local path to the repository') | ||
| parser.add_argument('parent_branch', help='git parent branch from which to branch') | ||
| parser.add_argument('branch', help='destination branch') | ||
| parser.add_argument('repository', default='./', nargs='?', help='local path to the repository') | ||
| parser.add_argument('parent_branch', nargs='?', help='git parent branch from which to branch') | ||
| parser.add_argument('branch', nargs='?', help='destination branch') |
There was a problem hiding this comment.
The default behavior when branch is not passed is not obvious. Is it a good idea to have that one optional?
There was a problem hiding this comment.
It uses the branch currently checked out in the repository.
There was a problem hiding this comment.
I see that, though:
- I kind of dislike the idea of making unchecked assumptions about the checked out branch ... very much especially if we advise people to
--push, and the previous behaviour, or even the new one when using--push, is to leavemasterbeing the current branch when done - if we still want it (and fix that inconsistency), it should appear in the param doc
Further more, since the idea is to get it run automatically soon, we don't gain much by making it optional.
There was a problem hiding this comment.
I don't think it's a good idea to make the branch optional. And I don't think it's really useful to make the current path the default.
There was a problem hiding this comment.
I get it that the CLI interface for this command is not optimal, but that's really one command for which we don't want people to spare some thinking before executing it. Hence the positional mandatory arguments.
One argument that is confusing is the "parent branch" argument, which could be removed if we updated the docs so that people created the branch by hand when it didn't exist yet. "parent branch" is an argument that could be a candidate for being optional. But then I think it would be more confusing than leaving it mandatory. If you're confused by the parameter, then you don't really know what the script does, so better not use it.
The merge to master option should go. I mostly used it back when I didn't go through a PR for such merges, and it is now useless.
Coming back to the initial branch after doing the work would be nicer than going to master.
There was a problem hiding this comment.
Well, when running git push, git makes an unchecked assumption that this is currently the right branch. It doesn't force us to provide the branch name, even though we could push something wrong. It looks the same to me.
But yes, let's run it automatically, and we won't have to provide anything on the command line 🙂
| print(" switching to master before leaving...") | ||
|
|
||
| call_process(['git', 'checkout', 'master']) | ||
| if args.branch: | ||
| print(" switching to master before leaving...") | ||
| call_process(['git', 'checkout', 'master']) |
There was a problem hiding this comment.
That "switching to master before leaving" is indeed a definitely bad idea in itself. It could make sense to restore what the current branch is in the end, OTOH.
But then, there is that "merging to master" below, which then does require switching to master (which is not necessarily "switching back").
There was a problem hiding this comment.
I would prefer that too. But so far, I've done the branch management by hand, so I can't really say.
the repository path defaults to .
the local branch is used if not provided on the command line