-
Notifications
You must be signed in to change notification settings - Fork 1.2k
Check if the clone target directory already exists. #369
Conversation
.IfTrue(x => x.Length > 200, "Path too long") | ||
.IfContainsInvalidPathChars("Path contains invalid characters") | ||
.IfPathNotRooted("Please enter a valid path") | ||
.IfTrue(IsAlreadyRepoAtPath, Resources.RepositoryNameValidatorAlreadyExists); |
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.
Instead of copying this here, maybe it's best to pass an observable to CreateBaseRepositoryPathValidator
. The repository creation viewmodel also does these exact same checks, it would be nice not to duplicate code.
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 looked into that, but it was just different enough to cause problems so I gave up and did it like this. I can look into it again though.
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 only two differences are that this one takes an observable and the other one makes its own observable, so we can just pass in one instead of letting it do that; and that there's an extra IsTrue
directive on this one, which can be added to the extension method since both create and clone should be doing the same validations. Everything else is the same, am I missing something?
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 issue was that in the create view model the repository name is validated separately from the base path, whereas in the clone dialog we don't need to validate the name as it's already coming from GitHub. So the options were either merge the two validators in the create view model somehow or have two validators for a single textbox in clone. Or just do as I did, and not share the validation code. If that makes sense?
As we decided that it was clearer to write out validation conditions explicitly in the VM.
✨ |
And show a validation error if it doesn't. Fixes #360.