-
Notifications
You must be signed in to change notification settings - Fork 1.3k
api: identify local repos with os.path.exists #3166
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
| @contextmanager | ||
| def _make_repo(repo_url, rev=None): | ||
| if not repo_url or urlparse(repo_url).scheme == "": | ||
| if not repo_url or os.path.exists(repo_url): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm using a solution suggested by @efiop , which identifies local repositories by checking if they are in the file system. There's no way a URL (e.g. https://, git@github) could be missed like that.
I'm not aware of any other edge case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably, file:// but no one uses it 😅
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, we don't support file:// anywhere else, so no need to worry about it for now. Neither any user asked for it ever 🙂
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Possibly could use a test, but this solution so neat and trivial that it doesn't require it 👍
|
It'd be good to have a regression test so that ssh URLs don't get broken later on? 🙂 Not related to this change though, but regarding the issue. Thoughts? |
|
sure, @skshetry , didn't know how to test it properly, to be honest 😅 Any ideas? Do you want to send the pull request? |
|
@skshetry The |
Close #3111