-
Notifications
You must be signed in to change notification settings - Fork 30
fix: restore insist #759
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
fix: restore insist #759
Conversation
|
Warning Rate limit exceeded@TobiTenno has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 10 minutes and 18 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (2)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds a new exported utility Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR restores the insist utility function, which validates that an object has required properties. The implementation includes a new integrity module with the function, tests, and an export from the main utilities file.
Key changes:
- Adds
insist()function for validating required object properties with TypeErrors - Includes basic test coverage for the function
- Exports the integrity module from tools/utilities.ts
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| tools/integrity.ts | New file implementing the insist() validation function |
| test/utilities/integrity.spec.ts | Test file for the integrity module |
| tools/utilities.ts | Adds export for the new integrity module |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/utilities/integrity.spec.ts (1)
1-9: Tests cover core behavior; consider re-export path + edge casesThe tests exercise the main behaviors of
insist(empty object, missing key, all keys present), which is enough to validate the basic contract.Two optional improvements you might consider:
Import via the utilities entrypoint
Sincetools/utilities.tsnow re-exports from./integrity, you could import from../../tools/utilitiesinstead. That would also verify the public re-export path works as expected, not just the direct module.Add an explicit falsy-value case if needed
Given the current implementation, a property with value0,'', orfalsewill causeinsistto throw (because the check is truthiness-based). If you intend those to count as “present”, an extra test (e.g.insist({ a: 0 }, 'a')) would help lock in the desired semantics and drive any implementation tweak intools/integrity.ts.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
test/utilities/integrity.spec.ts(1 hunks)tools/integrity.ts(1 hunks)tools/utilities.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
test/utilities/integrity.spec.ts (1)
tools/integrity.ts (1)
insist(6-16)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Upload results
🔇 Additional comments (1)
tools/utilities.ts (1)
3-5: Re-export of./integrityaligns with existing utilities patternAdding
export * from './integrity';is consistent with this file’s role as an export aggregator and cleanly exposesinsistthrough the central utilities entrypoint. Just be aware this expands the public API in line withtimeDateandtranslation, which seems intentional for a “restore insist” bugfix.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
test/utilities/integrity.spec.ts (1)
6-17: Excellent test coverage! All past feedback addressed.The test suite now comprehensively covers the edge cases that were previously missing:
- Falsy but valid values (0, false, '')
- null as the first argument
- No required properties
All test assertions correctly validate the expected behavior of the
insistfunction.Optional: Consider adding a test for
undefinedvalues to make the coverage even more explicit:+ (() => insist({ a: undefined }, 'a')).should.not.throw();This would explicitly verify that keys with
undefinedvalues are treated as present (since the implementation uses theinoperator which checks key existence, not value).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
test/utilities/integrity.spec.ts(1 hunks)tools/integrity.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- tools/integrity.ts
🧰 Additional context used
🧬 Code graph analysis (1)
test/utilities/integrity.spec.ts (1)
tools/integrity.ts (1)
insist(6-16)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Upload results
- GitHub Check: Summary
🔇 Additional comments (1)
test/utilities/integrity.spec.ts (1)
1-4: LGTM! Past review feedback addressed.The
should()initialization has been correctly added as suggested in the previous review.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
🎉 This PR is included in version 3.1.12 🎉 The release is available on: Your semantic-release bot 📦🚀 |
What did you fix?
restore insist
Reproduction steps
Evidence/screenshot/link to line
Considerations
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.