Conversation
There was a problem hiding this comment.
Hi, this is amazing implementation and I like how you technically handled both flag parsing and tests 🏆
However, I would really really prefer if we did not involve any knowledge of git clone flags; i.e. if we avoided parsing those flags altogether. This is mostly for ease of maintenance and for forward-compatibility with future git.
How about we accept a 2nd argument to gh repo clone?
gh repo clone owner/repo→ clones torepogh repo clone owner/repo dest→ clones todestgh repo clone <repo> [<dir>] -- [<flags>...]→ additional flags togit clonethat do NOT include the destination
I think this would be a way simpler implementation, too.
thefotios
force-pushed
the
respect-clone-target
branch
from
d079f13 to
59f239b
Compare
|
Ah, yea. The future compatibility is why I was thinking about using their parser, but your suggestion is 💯 better. I left |
|
@mislav this is ready for re-review |
mislav
left a comment
mislav
left a comment
There was a problem hiding this comment.
This looks great! The approach looks solid to me ✨
command/repo.go
Outdated
| Long: `Clone a GitHub repository locally. | ||
|
|
||
| To pass 'git clone' options, separate them with '--'.`, | ||
| To pass 'git clone' options, separate them with '--'. |
There was a problem hiding this comment.
Maybe we should say "flags" instead of "options" here, so that people don't think that all git clone options are accepted after the -- (<repo> and <directory> are not)?
There was a problem hiding this comment.
Good call. The only options that git clone accepts are the repo and directory; everything else are flags. So this now has effectively the same function signature (except that flags have to come after the --).
Fixed in 199021d
command/repo.go
Outdated
| To pass 'git clone' options, separate them with '--'.`, | ||
| To pass 'git clone' options, separate them with '--'. | ||
|
|
||
| In order to clone to a specific directory, provide it before the '--' instead of after the other additional options.`, |
There was a problem hiding this comment.
Hmm let's avoid adding this. It's technically true, but it makes help text harder to grok, and it should be obvious from the usage synopsis that <directory> should come before the --.
command/repo.go
Outdated
| RunE: repoView, | ||
| } | ||
|
|
||
| func parseExtraArgs(extraArgs []string) (args []string, target string) { |
There was a problem hiding this comment.
Nit: this function has a generic-sounding name, but applies specifically to parsing clone arguments (it makes little sense otherwise). Since this function is shared across all command package, which is our biggest package, we could benefit from it having a more specific name, such as parseCloneArgs
There was a problem hiding this comment.
Ah right, go newb mistake ;) Fixed in 6428e3d
|
@mislav Thanks for the feedback; I made the requested changes if you want to resolve the conversations. |
|
👋 @thefotios there's a familiar face! 😸 |
3 participants
Fixes #653
This allows a user to specify the target directory to clone to as part of the additional arguments.
It's not the prettiest implementation, but it works. It essentially parses the extra args to find any orphaned ones, Ideally we could somehow leverage git's actual command line arg parsing, maybe through something like this. But this is pretty much my first go code, so I wasn't really sure how to go about doing that.
Based on conversation in #721, this functionality was separated into it's own feature.