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

Remove CollectionContainsConstraint #1324

Closed
CharliePoole opened this issue Mar 7, 2016 · 4 comments · Fixed by #2384
Closed

Remove CollectionContainsConstraint #1324

CharliePoole opened this issue Mar 7, 2016 · 4 comments · Fixed by #2384

Comments

@CharliePoole
Copy link
Contributor

See the note at the bottom of this page.

We could do away with the constraint and have all the syntactic elements generate a SomeItemsConstraint. It would remove one small class and a test class from our codebase.

OTOH, if there are people who actually construct the constraint, it would break their code. There can be other things like this under Constraints, so let's decide on whether to keep such code or remove it.

@CharliePoole CharliePoole added this to the Ideas milestone Mar 7, 2016
@rprouse
Copy link
Member

rprouse commented Mar 8, 2016

Would the plan be to use SomeItemsConstraint in Has.Member(), Contains.Item() and Does.Contain() instead?

I doubt many people use our constraints directly. Even then, should we deprecate for one release?

@CharliePoole
Copy link
Contributor Author

Even simpler: return Has.Some.EqualTo(expected)

I think there's a benefit to eliminating any unneeded constraints, since I often find my self searching through and comparing those that work in a similar way. It's a time sink.

The trick would be to deprecate the constructor of the constraint while avoiding deprecating Has.Member(), etc. Perhaps by adding an internal static factory method for use by the syntax elements. Or we could just not worry about it and remove. :-)

@CharliePoole CharliePoole modified the milestone: Ideas Jun 24, 2016
@CharliePoole
Copy link
Contributor Author

@rprouse We already discussed this. Shall we move it to Backlog at a low priority?

@rprouse
Copy link
Member

rprouse commented Nov 20, 2016

I say just not worry and remove.

mikkelbu added a commit to mikkelbu/nunit that referenced this issue Jun 10, 2017
There is some code duplication between
`DictionaryContainsKeyConstraint` and
`DictionaryContainsValueConstraint`, but
not much will be saved by extracting
`Matches` and `Using` to a helper class.

Fixes nunit#1324
@rprouse rprouse added this to the 3.8 milestone Aug 26, 2017
@rprouse rprouse changed the title Remove CollectionContainsConstraint? Remove CollectionContainsConstraint Aug 26, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants