Add RequireThat

[james-w]

No description provided.

[claude]

Code Review Summary

This PR adds RequireThat() - a fatal assertion function that stops test execution on failure. Implementation is solid with good docs and tests.

Strengths

• Clear API design mirroring AssertThat
• Comprehensive documentation with examples
• Good test coverage
• Clean property test logging with logPassthrough mechanism

Issues Found

1. Bug: Error reporting order in RequireThat (matchers.go:100-123)

Details logged BEFORE main message, but AssertThat logs them AFTER:

• AssertThat: main message → details
• RequireThat: details → main message (inconsistent)

Fix: Log main message with Errorf(), then details, then Fatalf("") to stop.

2. Helper() call in generator.go:29

T.Helper() is intentionally a no-op (property.go:52), so calling it with comment "Mark as test helper" is misleading. Either remove it or keep for interface consistency.

Minor Suggestions

• Add comment explaining buggySqrt is intentionally buggy (example/property_logging_test.go:26)
• Mention *T satisfies TestingT interface (matchers.go:33)

Pre-merge

• Run pls run ci-full
• Verify API docs up-to-date
• Consider README.md update for new public API

Summary

Strong PR. Fix error reporting order bug in RequireThat. Property test logging enhancement is particularly nice.

[github-advanced-security]

This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation.

[claude]

Review

This PR adds RequireThat() - a well-implemented fatal assertion function. The code is solid with good tests and comprehensive documentation.

Code Quality: ✓

Implementation (matchers.go:88-128)

• Clean API mirroring AssertThat - good consistency
• Proper error reporting order: main message → details → Fatalf (matches AssertThat pattern)
• Correct use of t.Helper() and AST expression capture
• Empty string in Fatalf("") is appropriate since errors already reported via Errorf

Tests (matchers_test.go:328-369)

• Good coverage: success case, Fatalf verification, error reporting, details handling
• Proper use of testlib.Spy for testing fatal behavior
• Test at line 357 verifies detail ordering correctly

Supporting Changes

• testlib.Spy.Fatalf properly handles empty format string (lines 44-54)
• Minor formatting: alignment fix in property.go:17 (logPassthrough field comment)
• Import ordering fixes per goimports standard

Notes

1. Re: existing review comment - The mentioned bug about error reporting order doesn't exist in the current code. RequireThat logs main message first (lines 109/117), then details (121-123), matching AssertThat's behavior exactly.

2. TestingT interface expansion - Adding Fatalf() to the interface is the right design choice. Both *testing.T and *testing.B already implement it.

3. Documentation - Excellent additions to introduction and core-matchers docs with clear examples of when to use each assertion style.

Pre-merge Checklist

Per CLAUDE.md, run:

• pls run ci-full - Full CI verification
• Verify generated API docs are current (pls run check-api-docs)

Summary

Approve - Clean implementation, good tests, excellent documentation. No issues found.

[claude]

Code Review

This PR adds RequireThat() - a well-designed fatal assertion function. The implementation is clean and well-tested.

Code Quality ✓

Implementation (matchers.go:88-128)

• Correct implementation: messages logged via Errorf(), then Fatalf("") to stop
• Proper code reuse: DRY with AssertThat - only difference is the final Fatalf call
• Good use of t.Helper() and AST expression capture
• Empty string in Fatalf("") is correct since errors already reported

Tests (matchers_test.go:328-370)

• Comprehensive coverage: success case, Fatalf behavior, error reporting, details handling
• Proper use of testlib.Spy for testing fatal behavior without stopping outer test
• Tests verify the critical requirement: errors logged BEFORE Fatalf

Documentation

• Excellent additions to introduction and core-matchers docs with clear examples
• Good API reference updates
• Well-explained distinction between AssertThat and RequireThat use cases

Supporting Changes

• testlib.Spy properly implements Fatalf with empty format handling (lines 44-54)
• Import ordering fixes consistent with goimports
• Minor formatting alignment in property.go

Notes

The earlier review comment about error reporting order was based on outdated information. The current implementation correctly logs the main message first (lines 109/117), then details (121-123), matching AssertThat's behavior exactly.

Pre-merge

Per CLAUDE.md, run:

• pls run ci-full - Full CI verification
• Check README.md - consider mentioning RequireThat in quick start

Summary

Approve - Clean implementation, comprehensive tests, excellent documentation. Ready to merge after CI passes.