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

Requires.NotEmpty(Guid) #49

Closed
scottdorman opened this issue Sep 28, 2020 · 7 comments
Closed

Requires.NotEmpty(Guid) #49

scottdorman opened this issue Sep 28, 2020 · 7 comments

Comments

@scottdorman
Copy link
Contributor

The Requires.NotEmpty(Guid) method here was replaced with Requires.NotDefault, which creates an inconsistency with Microsoft.VisualStudio.Validation. Should we update that repo to match? This is tangentially related to #38.

@AArnott
Copy link
Owner

AArnott commented Sep 28, 2020

The Requires.NotEmpty(Guid) method here was replaced

I can't find any evidence that that API used to be in this library. At least as of 2.4.22, it seems the Requires class didn't have that method. The vs-validation library does define that method.
Of the two, I prefer NotDefault<T> because it works for more parameter types. Are you proposing that we add NotDefault<T> to vs-validation or that we add NotEmpty(Guid) to this repo?

@scottdorman
Copy link
Contributor Author

Correct, NotEmpty(Guid) never existed here. I also prefer NotDefault<T>. I was thinking that NotDefault<T> should probably be added to vs-validation and have NotEmpty(Guid) be removed. I put the issue here as I wasn't sure which direction you might want it to go (vs-validation => here or here => vs-validation).

@AArnott
Copy link
Owner

AArnott commented Sep 28, 2020

and have NotEmpty(Guid) be removed

That's never going to happen. vs-validation is used in many Microsoft components and thus has a high backward compat obligation.
I don't mind copying NotDefault<T> from here to vs-validation, but that's probably all I'd want to do.

@scottdorman
Copy link
Contributor Author

As I was looking at vs-validation for the other PR, I noticed that it's not just NotDefault<T> which is missing. It's also missing That, ValidState, and the new ValidElements<T> methods. Should those get pulled in as well (along with their associated tests)?

@AArnott
Copy link
Owner

AArnott commented Sep 30, 2020

I'm in favor of copying the following to vs-validation: ValidElements<T>, NotDefault<T>.

Let us not copy over ValidState (which is redundant with Verify.Operation which is the better API for it). We should [Obsolete] the ValidState API with a message telling folks to use Verify.Operation instead.

That is similarly redundant with Requires.Argument and should be deprecated. Having redundant methods for the sake of sounding "fluent" makes an API more confusing, IMO.

@scottdorman
Copy link
Contributor Author

scottdorman commented Sep 30, 2020

That all makes sense. I'll have a PR shortly for that. Should we also add [Obsolete] to NotEmtpy(Guid) or should it simply be a call to NotDefault?

@AArnott
Copy link
Owner

AArnott commented Oct 2, 2020

Let's leave NotEmpty(Guid) alone. It's in vs-validation and we won't be obsoleting it there.

AArnott added a commit that referenced this issue May 2, 2023
AArnott added a commit that referenced this issue May 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants