Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
gto show --repo accepts remote repositories #269
gto show --repo accepts remote repositories #269
Changes from 25 commits
d984781
ddfe434
e533d2d
feea54e
2d79391
b5e9810
b188057
2297ac5
4dd89ed
76aebe2
4686711
74e0493
7af727a
3d2d970
fcb55d0
6415782
8dfa922
c1dbcaa
ec0c7eb
0293e67
3f9cd51
fe29fa0
b2475e7
f548a3c
8464ec0
35eeb93
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
Let's merge these functions and test a single one only? Now having
git_clone
as a separate seems redundant. You can easily testcloned_git_repo
without generating a temporary directory additionally in tests.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.
Can gladly do it for the tests (actually it connects with something I wanted to discuss about public/private functions -> will add it below), but for the functions I would suggest to leave them as they are, again to leave the door open for a different engine. It doesn't do any harm and makes the separation clear: this is the only thing you need to change if you want to use a different git engine.
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.
Thanks, I checked out the second PR you created.
My opinion:
I think the PR is ready, no need to overthink this. We can get fix all of that when that would be needed. Let's merge this and move along. WDYT?
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.
fine with me! :) let's do that
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.
The same q as above, does it make sense to keep these in
conftest.py
? Except for introducing one more file. @mike0sv ?