-
-
Notifications
You must be signed in to change notification settings - Fork 43
Exit gracefully when no meeting found instead of throwing error #203
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
Exit gracefully when no meeting found instead of throwing error #203
Conversation
Co-authored-by: avivkeller <38299977+avivkeller@users.noreply.github.com>
Co-authored-by: avivkeller <38299977+avivkeller@users.noreply.github.com>
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 refactors error handling in the calendar module by converting findNextMeetingDate to return null instead of throwing an error when no meeting is found. The change introduces proper test coverage and moves error handling responsibility to the caller.
- Refactored
findNextMeetingDateto returnnullinstead of throwing an error when no meeting is found - Added comprehensive unit tests for the
findNextMeetingDatefunction - Updated test scripts in package.json to use Node.js built-in test runner discovery
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| test/calendar.test.mjs | New test file with comprehensive test coverage for findNextMeetingDate function, including edge cases for null returns |
| src/calendar.mjs | Exported getWeekBounds function and changed findNextMeetingDate to return null instead of throwing an error |
| package.json | Simplified test scripts to use Node.js automatic test discovery instead of explicit glob patterns |
| create-node-meeting-artifacts.mjs | Added null check after calling findNextMeetingDate to gracefully handle cases where no meeting is found |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ovflowd
left a comment
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.
SGTM!
When no meeting is found in the calendar (common for bi-weekly meetings), the script threw an error. Changed to exit with code 0 and log an informative message.
Changes
src/calendar.mjs:findNextMeetingDate()returnsnullinstead of throwing. ExportedgetWeekBounds()for reuse.create-node-meeting-artifacts.mjs: Check for null meeting date, log message, exit 0.test/calendar.test.mjs: Added tests covering null return scenarios.package.json: Fixed test glob to include root-level test files (test/*.test.mjs).Behavior
Original prompt
💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.