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

move size check out of Iterable.contains assertions #299

Closed
robstoll opened this issue Jan 10, 2020 · 11 comments · Fixed by #768
Closed

move size check out of Iterable.contains assertions #299

robstoll opened this issue Jan 10, 2020 · 11 comments · Fixed by #768
Assignees
Milestone

Comments

@robstoll
Copy link
Owner

robstoll commented Jan 10, 2020

Platform (jvm, js, android): all
Extension (none, kotlin 1.3, jdk8): none

Code related feature

Discussion see #292 and robstoll/atrium-roadmap#53

In short move the > size: 2 to be ... check out of contains..

@ivanmiklec
Copy link
Collaborator

I'll work on this

@robstoll
Copy link
Owner Author

robstoll commented Jul 1, 2020

@ivanmiklec Let me know in case you need more information

@ivanmiklec
Copy link
Collaborator

ivanmiklec commented Aug 16, 2020

@robstoll was busy but I'm going to solve this during the upcoming week

@robstoll
Copy link
Owner Author

@ivanmiklec No worries, I don't expect it to be part of 0.13.0 so no rush.

@robstoll
Copy link
Owner Author

robstoll commented Sep 3, 2020

@ivanmiklec I am going to refactor IterableContains, I suggest you wait with your implementation until #564 is closed. If you have already started: don't worry, the change will mainly be moving code from domain-robstoll to logic

@ivanmiklec
Copy link
Collaborator

Sure, going to wait a bit then

@robstoll
Copy link
Owner Author

@ivanmiklec refactoring is merged, you need to modify the files in /logic/atrium-logic-common/src/main/kotlin/ch/tutteli/atrium/logic/creating/iterable/contains/creators/impl

@ivanmiklec
Copy link
Collaborator

Hey, just to check am I on a good path for this one and to pick up a few clues, found size check Assertion is being added in InAnyOrderOnlyAssertionCreator and InOrderOnlyBaseAssertionCreator. For now I removed it from there, don't know if that is Ok.

@robstoll
Copy link
Owner Author

Writing from my mobile (without code at hand). I think the size checks should still be done in there but the resulting assertion at the end of the methods needs to re-arranged. First the size check and then contains.
Now... I remember that I have factored out some code into an abstract class. Might be that you need to make the adjustment there. Let me know if this already helps. Otherwise, I am going to look into the code at some point

@ivanmiklec
Copy link
Collaborator

Yeah thanks for elaborating, finally understood idea, found there is size assertions being added in kotlin/ch/tutteli/atrium/logic/creating/iterable/contains/creators/impl/InAnyOrderOnlyAssertionCreator.kt, still not kinda sure where to go with it

@robstoll
Copy link
Owner Author

robstoll commented Nov 2, 2020

Line 57 ff. we create a feature assertion for size and add it to assertions => don't do this any more, instead we want to add the feature assertion on the same level as the summary assertion. I.e. assertionBuilder.invisibleGroup

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.

2 participants