The Wayback Machine - https://web.archive.org/web/20200705081409/https://github.com/github/VisualStudio/pull/2395
Skip to content
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

Refactoring RepositoryForm Validators #2395

Open
wants to merge 1 commit into
base: essentials-publish
from

Conversation

@StanleyGoldman
Copy link
Contributor

StanleyGoldman commented Jun 27, 2019

No description provided.

await SafeRepositoryNameWarningValidator.ResetAsync();
RepositoryName = name;
});
// this.WhenAny(x => x.SelectedConnection, x => x.SelectedAccount,

This comment has been minimized.

Copy link
@jcansdale

jcansdale Jun 28, 2019

Collaborator

We should delete these comments!

@@ -25,8 +25,8 @@ public interface IRepositoryForm : IViewModel
/// dashes.
/// </summary>
string SafeRepositoryName { get; }
ReactivePropertyValidator<string> RepositoryNameValidator { get; }
ReactivePropertyValidator<string> SafeRepositoryNameWarningValidator { get; }
ReactivePropertyValidator<(string repositoryName, IConnection connection, IAccount account)> RepositoryNameValidator { get; }

This comment has been minimized.

Copy link
@jcansdale

jcansdale Jun 28, 2019

Collaborator

Looking at the code, I can't see where we're using the connection or account for validation. I wonder if once upon a time we were checking for an existing repository with the same name as part of the validation? It looks like this check is now done and the error surfaced when the user attempts to create the repository.

I think we might be able to simplify this to use just the repositoryName. I'm wondering if the buggy code was actually completely obsolete. ;-)

Copy link
Collaborator

jcansdale left a comment

I'm wondering if we could simply remove the buggy code?

See comment:
#2395 (review)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants
You can’t perform that action at this time.