-
Notifications
You must be signed in to change notification settings - Fork 728
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 #2239
Remove CollectionContainsConstraint #2239
Conversation
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
Sorry we haven't looked at this yet, we've been busy working in other areas. I have self assigned myself as a reviewer so I won't forget. I am off on vacation this afternoon. Hopefully I will find time while I am away. Other @nunit/core-team or @nunit/framework-team team members, feel free to review. You don't have to wait for me. |
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'm all for cleaning things up, but what if end users have code that uses CollectionContainsConstraint
? Shouldn't we at least obsolete it for a while first?
No need to be sorry, the PR is not that old (and I've been waiting a lot longer than 1-2 weeks for a review in other repositories 😄 ). Wrt. whether we should obsolete-mark {{CollectionContainsConstraint}} or not, I will let the teams decide. We can always keep the PR and apply it later. |
I missed this comment: #1324 (comment) Looks like the decision is to remove. 👍 |
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.
Hey @mikkelbu. Sorry it took me a while to get round to looking at this one. Looks mostly good to me - just one point we need to address with CollectionContainsConstraint.Using
, that I've commented on below.
I agree with your point about duplication between DictionaryKey/ValueContains - it's not worth worrying about.
Given there's no time urgency on this one at all, I'd personally prefer to see us [obsolete]
the CollectionContainsConstraint constructor in v3.8, and then merge this into 3.9. I doubt this change will affect many people, but I given this isn't blocking anything, I think we can afford to go about it 'the nice way'. 😄
@nunit/framework-team - would also be good if someone would give this a second review before merging, there's a fair bit going on here, and I've never been that good at thinking through potential side effects! 🙂
/// </summary> | ||
/// <param name="comparison">The comparison function to use.</param> | ||
/// <returns>Self.</returns> | ||
public CollectionContainsConstraint Using<TCollectionType, TMemberType>(Func<TCollectionType, TMemberType, bool> comparison) |
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.
If we're going to do this as much as possible as a 'non-breaking change',wouldn't we need to add this Using
signature to EqualConstraint? I'm not totally keen, as it would clutter up EqualConstraint, but think it's necessary to avoid breaking any existing tests.
i.e. Consider the below scenario, which would work with the CollectionContainsConstraint, but would no longer compile after these changes.
@nunit/framework-team - someone double check my logic, I'm not missing something here, am I?
@mikkelbu - if I'm right, then looks like we're missing a test for this case! Would you mind adding one when you come back to make the changes?
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.
@ChrisMaddock I think you are correct.
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.
@ChrisMaddock, I think you are correct. @mikkelbu, would you mind making an update to address this? Other than that, looks good.
@ChrisMaddock You seem pretty darn good at thinking through potential side effects to me! As far as I can see, that's the only breaking change besides removal of the class itself. All APIs on CollectionContainsConstraint must be moved to EqualConstraint. |
Added overload of Using to EqualConstraint to ensure that it exposes the same API as CollectionContainsConstraint. Also added a test of the overload.
@nunit/framework-team I've reintroduced CollectionContainsConstraint (as it is on master) and I've added the Using overload to EqualConstraint (and a test thereof). Since there are Conflicting files I'm a bit unsure what to do (I'm primarily a mercurial user and need to look up most "advanced" git commands on stackoverflow 😄). I guess that I can rebase my topic branch on top of master and then do a force push, but is that the normal approach in this project (and how do I do it). |
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.
Changes look good!
Yes, force pushing is the normal approach. I'm also a mercurial user. 😄 So don't take this as gospel - but I normally do something a bit like this. There's likely a better way!
Edit: Actually, are you using a fork, rather than a branch in the main repository? In that case, it's more complicated - and I may defer to someone else to advise! |
Yes. I'm using a fork. I had no idea that it would make it more complicated. From what I can read in https://robots.thoughtbot.com/git-interactive-rebase-squash-amend-rewriting-history the only difference is just that I need to rebase on top of |
@mikkelbu - yep, that bottom list from your link looks sensible. 😄
|
@mikkelbu Yeah, I don't even allow a |
I see the point on forks, but if you have master locally, it can be very convenient, especially if you are offline some of the time. Of course, you have to keep it up to date. @mikkelbu You're a committer so you can make branches locally rather than in a fork, you know. |
@CharliePoole You always have upstream/master locally when you |
If it's my own project, as opposed to somebody else's that I'm forking, I like to have a working directory for master because it's easier to scan through the code that way. |
That makes sense. I haven't had experience forking any of my own projects yet. |
I think you missed that I was making a distinction between forking and working on your own project directly. |
Oh, I did. |
@mikkelbu this looks good. I will resolve the conflicts and merge. |
Thanks @rprouse 👍 |
There is some code duplication between
DictionaryContainsKeyConstraint
andDictionaryContainsValueConstraint
, butI feel that not much will be saved by extracting
Matches
andUsing
to a helper class.Also I'm a bit unsure when to use
Has.Some.EqualTo
ornew SomeItemsConstraint(new EqualConstraint(expected))
.Fixes #1324