Accept common URLs as repo spec in parse_github_repo_spec(); closes #90 #108
Conversation
R/install-github.R
Outdated
|
|
||
| parse_github_repo_spec <- function(repo) { | ||
|
|
||
| repo <- gsub("\\.git$", "", repo) |
There was a problem hiding this comment.
I would only remove the .git suffix with the prefixes, because some git urls might not handle the .git-less form, like GitHub.
There was a problem hiding this comment.
OK I can do that. But just to check ... what are you actually worried about? That there might be a .git I should not strip? That there might be a GitHub repo actually named foo.git and jenny/foo.git should return username = jenny and repo = foo.git?
|
Thanks! Otherwise it looks great, I have wanted this for a long time! |
|
You can submit the exact same PR if you want, and I'll be online to request the change. :) |
|
and a bullet point in NEWS... |
|
See what you think now. I don't immediately see why travis r-devel is failing now but passed initially for this PR. Update: I restarted that job and then it passed. |
| @@ -1,6 +1,9 @@ | |||
|
|
|||
| # 1.1.0 | |||
|
|
|||
| * Accept browser, HTTPS, or SSH URL in GitHub repo specification, | |||
There was a problem hiding this comment.
What do you mean by "browser"?
There was a problem hiding this comment.
The URL that appears in your browser when you visit a repo. I frequently capture those on the clipboard. Maybe it's just a habit of mine?
If I've managed to make that clear ... can you think of a better term?
There was a problem hiding this comment.
But how is that different from just a HTTPS url?
There was a problem hiding this comment.
Or any URL within the repo is supposed to work? Like this one works as well: https://github.com/r-lib/remotes/pull/108?
|
Actually, you are right, chopping off the So feel free to submit the original or the updated form. |
I think it does feel more contained the way it is now, so better. I will close this and make a fresh one sometime between 7:30 and 8:10pm tonight. But I'm still going to point to this, because it's ended up being a good example of a PR process! |
|
Sorry, one more question. URLs like |
|
No that would currently give the wrong result. The only "browser URL" that explicitly works is the main one library(remotes)
parse_github_repo_spec("https://github.com/r-lib/remotes/pull/108")
#> $username
#> [1] "r-lib"
#>
#> $repo
#> [1] "remotes"
#>
#> $subdir
#> [1] "pull/108"I thought about this. There are many forms (pull requests, releases, etc.) but didn't want to tackle all of them. I still think the current addition is worth it but, yeah, don't want to overpromise re: browser URL support. |
|
How about just cutting off that part? |
|
At least for now? |
|
Maybe I should just mention the HTTPS and SSH URLs in the help and let the other be an undocumented things that works. We are typing at same time, but is that basically what you mean? |
|
I mean, I would just cut off the rest of the URL, i.e. Although this is not a good idea if we want to implement sg special for the various urls. OK, I think it would be better to give an error in this case. So basically, the URL has to end with the repo name. You don't need to do this for today, but I think in the long term it would make sense. |
|
Got it. |
I've added this, i.e. the only browser-style URLs that work are the HTTPS remote URL and the top-level URL. Otherwise, they error. I also made it work for GitHub enterprise URLs, since I see that How does this look? |
|
Great! Thanks much! |
|
Taken care of, as planned, in #109. |
PLEASE DO NOT MERGE THIS. As discussed, I'd like to make this PR live from my R-Ladies thing tomorrow night. But let's discuss if this solution is OK. Once true, I will close this and make the real PR then. I just did it in the simplest way, w/o disturbing the world's scariest regex just below 😃