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

AssertRelated expects a count of 1 #116

Open
dmfs opened this issue Oct 10, 2017 · 3 comments
Open

AssertRelated expects a count of 1 #116

dmfs opened this issue Oct 10, 2017 · 3 comments

Comments

@dmfs
Copy link
Owner

dmfs commented Oct 10, 2017

AssertRelated always expects a row count of 1, sometimes more than just one related row might exist (i.e. like when creating a recurring task or event).

@dmfs
Copy link
Owner Author

dmfs commented Dec 30, 2017

I've noticed that this can easily be overcome by decorating with Counted like so

new Counted(5, new AssertRelated(…));

The question is, is it good design to let AssertRelated set an expected count? Should we just remove this and let the developer apply Counted himself (possibly with a special decorator called Sole which represents the special case new Counted(1, …))?

@lemonboston
Copy link
Contributor

I suppose it can be okay in multiple ways.

How about BulkAssertRelated with an expected count as ctor parameter?
Or just adding the parameter to AssertRelated?
This would be of course be motivated to allow the convenience to use these dedicated classes in tests without an extra decoration.

Design-wise, it's of course better if count is applied separately to keep it SRP, but since AssertRelated is already a composed class for convenience, I probably wouldn't be concerned about that in this case.

@dmfs
Copy link
Owner Author

dmfs commented Jan 3, 2018

There are probably only very few cases in which we would need to assert the existence of multiple related rows. At present I only need that for recurrence expansion. BulkAssertRelated would be merely a copy of AssetRelated with that additional parameter. That wouldn't make any sense. On the other hand, adding another parameter to AssertRelated ctors doesn't seem worth the hassle either. We would end up with 8 ctors with up to 6 arguments.

So in this case I'd be ok with adding documentation to make clear that AssertRelated sets the expected count to 1 (which is the most common use case) and explain that this can be overridden with a Counted decorator.

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