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

feat: Add each-assertions. #408

Merged
merged 17 commits into from
Mar 22, 2022
Merged

feat: Add each-assertions. #408

merged 17 commits into from
Mar 22, 2022

Conversation

yeldiRium
Copy link
Contributor

@yeldiRium yeldiRium commented Jan 24, 2022

This includes a major rewrite of the types and structure of the top
level assert.that function to achieve re-use of the existing assertions
and keep outward type-safety.

This unfortunately breaks the internal type-safety of assertthat and
makes maintenance of the combined assertions harder.

Hannes Leutloff added 5 commits January 24, 2022 12:21
The order of the assertions when building up the assert function was
wrong.
It is now ordered from less specific to more specific, so that the most
specific assertions are always used.
This includes a major rewrite of the types and structure of the top
level assert.that function to achieve re-use of the existing assertions
and keep outward type-safety.

This unfortunately breaks the internal type-safety of assertthat and
makes maintenance of the combined assertions harder.
Copy link
Member

@goloroden goloroden left a comment

Choose a reason for hiding this comment

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

Just two super-small issues in the documentation (oh, and: I think we need to update the LICENSE.txt file to 2022, which is not (yet) part of this pull request, don't we?)

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Hannes Leutloff and others added 2 commits February 10, 2022 13:26
Co-authored-by: Golo Roden <golo.roden@thenativeweb.io>
goloroden
goloroden previously approved these changes Feb 10, 2022
@goloroden
Copy link
Member

@yeldiRium Basically, I'm fine with this 👍
Had you seen my comment regarding the date in the license file?

Besides that, from my POV, we're good to go 😊

@yeldiRium
Copy link
Contributor Author

@goloroden I've seen it now and bumped the year to 2022 :)

goloroden
goloroden previously approved these changes Feb 10, 2022
@goloroden
Copy link
Member

@yeldiRium Thanks 😊

From my side, this is fine. I've approved the PR, but didn't want to merge it, because I did not know whether you wanted to also have feedback from someone else.

If you are fine with this, please feel free to merge. 😊

Copy link

@strangedev strangedev left a comment

Choose a reason for hiding this comment

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

Implementation wise, there's just one small thing. Regarding the API, I have my concerns, please see below.

Maybe changing the API as described in my comment will improve the type safety, too?

README.md Outdated Show resolved Hide resolved
lib/assertions/combined/CombinedAssertions.ts Outdated Show resolved Hide resolved
lib/assertions/combined/assertActualIsInstanceOf.ts Outdated Show resolved Hide resolved
lib/assertions/combined/assertActualIsThrowing.ts Outdated Show resolved Hide resolved
lib/assertions/combined/assertActualIsThrowingAsync.ts Outdated Show resolved Hide resolved
lib/assertions/combined/assertions.ts Outdated Show resolved Hide resolved
test/unit/wrapAssertionForIterableTests.ts Outdated Show resolved Hide resolved
strangedev
strangedev previously approved these changes Mar 22, 2022
@strangedev
Copy link

Awesome 😍

dotKuro
dotKuro previously approved these changes Mar 22, 2022
Copy link
Member

@goloroden goloroden left a comment

Choose a reason for hiding this comment

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

@yeldiRium The changes look really great 👍

However, the documentation is not yet up-to-date (although this is just a small change). Could you please adjust this?

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@yeldiRium yeldiRium dismissed stale reviews from dotKuro and strangedev via 361768e March 22, 2022 11:56
@yeldiRium yeldiRium merged commit 0bcaac6 into main Mar 22, 2022
@yeldiRium yeldiRium deleted the feat/each-assertions branch March 22, 2022 12:00
goloroden pushed a commit that referenced this pull request Mar 22, 2022
# [6.5.0](6.4.0...6.5.0) (2022-03-22)

### Features

* Add each-assertions. ([#408](#408)) ([0bcaac6](0bcaac6))
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.

4 participants