Use URLs to detect git-like repos rather than name#748
Conversation
R/gitlab.R
Outdated
There was a problem hiding this comment.
I'm not totally sure why the look ahead option didn't work here, but the gitlab.com URL that is in the fixture seemingly required me to do (www|api)?.?
There was a problem hiding this comment.
A super-nit here would be to move the . into the pattern, and also properly escape it so it's matching a literal . and not any character. E.g. ((www|api)\\.)?
There was a problem hiding this comment.
Ah, yes that's much better
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
R/restore.R
Outdated
There was a problem hiding this comment.
The main downside is that this will do the wrong thing for users with hosted / enterprise instances of these services, when they're accessible from non-default URLs.
There was a problem hiding this comment.
We could also simply look for the presence of remote_host along with maybe remote_repo?
There was a problem hiding this comment.
That makes sense to me.
There was a problem hiding this comment.
Done! I've also backed out the URL changes (those are probably better / slightly more flexible, but if they aren't needed here I don't want to increase the chance that there's a bug in there that could disrupt other things)
karawoo
left a comment
karawoo
left a comment
There was a problem hiding this comment.
I think it'd be good to add a test case for the remote_host/remote_repo case; otherwise LGTM
Co-authored-by: Kara Woo <karawoo@users.noreply.github.com>
|
Ok, I've added NEWS and will merge in a sec |
3 participants
I added some unit tests for the
isFromCranlikeRepo()function and ensured that other tests passed (locally at least). I did need to make the URL detectors slightly more flexible to match the test fixtures (though realistically the URLs in the fixtures probably could be updated to be more like real ones instead.resolves #747