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

Include owner in default path when cloning a repository #1906

Merged
merged 13 commits into from Sep 13, 2018

Conversation

Projects
None yet
4 participants
@jcansdale
Collaborator

jcansdale commented Sep 10, 2018

What this PR does

Rather than suggesting a flat folder structure when cloning a repository, suggest a folder structure that includes the repositories owner

  • Suggest a default clone location of DefaultClonePath/Owner/RepoName
  • Allow user to change DefaultClonePath portion of path (keep when new repo is selected)

How to test

Immediately change default clone path

  1. Change the DefaultClonePath before selecting a repository
  2. Select a repository
  3. Check that owner\repo has been added to path

Change directory name after selecting repository

  1. Select a repository
  2. Change name of repository in path
  3. Select another repository
  4. Check the owner\repo in path has changed

Remove repository name from path

  1. Select a repository
  2. Remove name of repository in path
  3. Select another repository
  4. Check the repo has been added to path

Remove owner and repository name from path

  1. Select a repository
  2. Remove owner and name of repository from path
  3. Select another repository
  4. Check that owner\repo has been added to path

Clear the path

  1. Clear the path
  2. Select a repository
  3. Check that default path has been restored and owner\repo added to path

Remove owner from path

  1. Select a repository
  2. Remove owner from path
  3. Select another repository
  4. Check that previous repository name isn't included in path

jcansdale added some commits Sep 10, 2018

Check owner is included in default repository path
Expect test to fail until functionality implemented.
Include owner in default clone path
Keep the base path consistent if user edits the fully qualified path.
Add tests for when user edits path
Check different combinations of repository and path changes.
@meaghanlewis

This comment has been minimized.

Contributor

meaghanlewis commented Sep 11, 2018

👋 @jcansdale I just had a look at how this works.

Question:
What was your motivation for adding this functionality? Do you usually have more structure in the way your organize your repositories? Just curious here 😄

Feedback:
Thanks for all the test scenarios! Those all work how you describe. A couple other scenarios that stuck out for me were:

  • removing the full path : select a repository, remove the path, select another repository the path is only populated as owner\repo
  • removing only the owner name select a repository, remove just the owner and keep the repo name, select another repository and notice the path gets appended with another owner\repo so the full path becomes default clone path\repo\owner\repo

Both of which are fine as they are since a user will likely select a new local clone path they're happy with if something undesired happened.

I think this works really nicely and will be helpful at providing more structure for those who have lots of repositories from different owners or organizations.

@jcansdale

This comment has been minimized.

Collaborator

jcansdale commented Sep 11, 2018

@meaghanlewis,

Thanks for taking a 👀

What was your motivation for adding this functionality? Do you usually have more structure in the way your organize your repositories? Just curious here 😄

Yup, I always organize my folders in a way that mirrors GitHub. I'll often clone a few repositories from the same owner. It's the only way I can keep any kind of order!

removing the full path : select a repository, remove the path, select another repository the path is only populated as owner\repo

It would be easy enough to reset back to the default clone location if the user removes the full path. I think if someone was to do this, that is what they'd be hoping for?

removing only the owner name select a repository, remove just the owner and keep the repo name, select another repository and notice the path gets appended with another owner\repo so the full path becomes default clone path\repo\owner\repo

I'll see if this can be fixed as well without any undesirable consequences. I think its okay for the reason you mentioned, but maybe it could be better. 😄

jcansdale added some commits Sep 11, 2018

Check restore DefaultClonePath when cleared
Test that if the target Path is cleared, DefaultClonePath will be used
as the base path on next selection.
Restore DefaultClonePath if Path is cleared
If Path is cleared, restore the DefaultClonePath on next selection.
Check when just owner has been deleted
Test that we don't duplicate the repo on next selection.
Handle when just owner has been deleted
Don't include old the repository name when next repository is selected.
@jcansdale

This comment has been minimized.

Collaborator

jcansdale commented Sep 11, 2018

@meaghanlewis,

I've fixed the two scenarios that you mentioned and added them to the list.

I've also fixed an an edge case where the owner and repository had the same name. Previously it would only delete the repository rather than both.

@jcansdale jcansdale changed the base branch from refactor/clone to master Sep 11, 2018

@jcansdale jcansdale requested review from grokys and meaghanlewis Sep 11, 2018

@meaghanlewis

This comment has been minimized.

Contributor

meaghanlewis commented Sep 11, 2018

This LGTM!

@grokys

grokys approved these changes Sep 13, 2018

Just one tiny thing, but other than that looks good!

@@ -114,12 +114,10 @@ public async Task Path_Is_Initialized()
public async Task Repository_Name_Is_Appended_To_Base_Path()

This comment has been minimized.

@grokys

grokys Sep 13, 2018

Contributor

Need to update test name.

jcansdale added some commits Sep 13, 2018

Correct test name
We're now adding the owner and repository name to the path.

@jcansdale jcansdale merged commit d0ae1d5 into master Sep 13, 2018

2 checks passed

continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@jcansdale jcansdale deleted the refactor/clone-include-owner-in-path branch Sep 13, 2018

@meaghanlewis meaghanlewis added this to the 2.5.6 milestone Sep 13, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment