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

renaming iterableAssertions to consistent to + infitinive naming schema #889

Merged
merged 9 commits into from
May 4, 2021

Conversation

robstoll
Copy link
Owner


I confirm that I have read the Contributor Agreements v1.0, agree to be bound on them and confirm that my contribution is compliant.

@robstoll robstoll requested a review from jGleitz April 28, 2021 19:41
because we rename containsNot to notToContain
Copy link
Collaborator

@jGleitz jGleitz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looking at the code, I take issue with toHaveNextAndAny and toHaveNextAndAll.

First, when reading the function, I first did not understand what they do. My mind tried to parse them as ‘to have: (next and all {})’, which makes no sense. Only after backtracking I noticed that it’s meant to be ‘(to have next), (and all {})’.

Second, I generally expect an expectation function to ‘do one thing’. Yes, there are expectation functions that do other implied checks, but that is different from one function doing two inherently different things.

I was going to suggest that we split the two and let users drop in the and if they want, i.e. toHaveNext.and.all {}. However, I remembered that you are afraid of users using all without realizing that it does not imply a toHaveNext check. I still don’t share that fear, I think an all check that passes for empty collections is entirely reasonable. However, I don’t want to re-start the discussion.

So I guess all I can do is give feedback that I find toHaveNextAndAny irritating. I have no better solution that would not involve splitting the toHaveNext and any check, though.

Repository owner deleted a comment from codecov bot Apr 28, 2021
@robstoll
Copy link
Owner Author

robstoll commented Apr 28, 2021

@jGleitz thanks for the feedback

Looking at the code, I take issue with toHaveNextAndAny and toHaveNextAndAll.

I should have mentioned in the description what I plan to do in a next PR. I realised that for Expect<Iterable> notToBeEmptyAnd... is misleading as it could be that Iterable had next but does not have any more because one element was already consumed by a next() call. Thus, I will add a notToBeEmptyAndAll/Any/None to collectionExpectations.kt in a next PR -- so far I preferred the shorter version over notToBeEmptyAndAllElements etc. due to the fact that notToBeEmptyAndNoElement would no longer contain None (see your comment robstoll/atrium-roadmap#93 (comment)).

Yet, seeing that you are confused if we leave out Element, is a bad sign. I'll see if I can come up with a better solution.


Edit: I think toHaveElementsAndAll would already be better, no? I think if we have this then I don't introduce notToBeEmptyAndAll in addition, toHaveElements is good enough also for collection IMO.

@codecov
Copy link

codecov bot commented Apr 28, 2021

Codecov Report

Merging #889 (439d4ee) into master (b6e5373) will increase coverage by 0.05%.
The diff coverage is 97.29%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #889      +/-   ##
==========================================
+ Coverage   90.82%   90.88%   +0.05%     
==========================================
  Files         408      410       +2     
  Lines        4056     4091      +35     
  Branches      211      211              
==========================================
+ Hits         3684     3718      +34     
- Misses        322      323       +1     
  Partials       50       50              
Flag Coverage Δ
bbc 83.23% <16.21%> (-0.62%) ⬇️
bc 83.13% <16.21%> (-0.62%) ⬇️
current 89.92% <97.29%> (+0.03%) ⬆️
current_windows 88.95% <97.29%> (+0.04%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
...m/api/fluent/en_GB/charSequenceContainsCheckers.kt 91.66% <ø> (ø)
.../atrium/api/fluent/en_GB/collectionExpectations.kt 100.00% <ø> (ø)
...creating/charsequence/contains/impl/StaticNames.kt 100.00% <ø> (ø)
...tteli/atrium/api/fluent/en_GB/featureAssertions.kt 100.00% <ø> (ø)
.../tutteli/atrium/api/fluent/en_GB/fun0Assertions.kt 100.00% <ø> (ø)
...teli/atrium/api/fluent/en_GB/iterableAssertions.kt 100.00% <ø> (ø)
...m/api/fluent/en_GB/iterableLikeContainsCheckers.kt 100.00% <ø> (ø)
...um/api/infix/en_GB/charSequenceContainsCheckers.kt 91.66% <ø> (ø)
...creating/charsequence/contains/impl/StaticNames.kt 100.00% <ø> (ø)
...tteli/atrium/api/infix/en_GB/iterableAssertions.kt 100.00% <ø> (ø)
... and 19 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update b6e5373...439d4ee. Read the comment docs.

@jGleitz
Copy link
Collaborator

jGleitz commented Apr 29, 2021

I think toHaveElementsAndAll would already be better, no?

Yes, I think so.

How will the version of all that does not require the collection to have elements be called like?

@robstoll
Copy link
Owner Author

I suggest notToHaveElementsOrAll, notToHaveElementsOrAny, notToHaveElementsOrNone

same same notToHaveNext as well as for toHaveNextAndAll/Any/None
@robstoll robstoll merged commit f442cda into master May 4, 2021
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

Successfully merging this pull request may close these issues.

2 participants