Skip to content
This repository has been archived by the owner on Jun 21, 2023. It is now read-only.

Refactoring RepositoryForm Validators #2395

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

Conversation

StanleyGoldman
Copy link
Contributor

No description provided.

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

@jcansdale jcansdale Jun 28, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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; }
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 jcansdale left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

See comment:
#2395 (review)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants