Fixed duration issue when having multiple time units #350
Conversation
wouterlemcke
force-pushed
the
durationFix
branch
from
1d2d611 to
b66b57b
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
Warning Rate limit exceeded@jens-maus has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 7 minutes and 48 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. WalkthroughThe changes made in this pull request primarily involve modifications to the Changes
Possibly related PRs
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- ical.js (1 hunks)
- test/test-async.js (1 hunks)
- test/test.js (1 hunks)
- test/test_date_time_duration.ics (1 hunks)
🔇 Additional comments (3)
test/test_date_time_duration.ics (1)
16-16: LGTM! Duration format is RFC 5545 compliant.The updated duration
PT1H15Mcorrectly represents a period of 1 hour and 15 minutes, following the iCalendar specification. This test case will help verify the fix for handling multiple time units in duration calculations.Let's verify other duration formats in the test suite:
✅ Verification successful
Duration format is valid and well-tested across the test suite
The test suite includes a good variety of duration formats that verify different scenarios:
- Simple durations:
PT1H- Complex durations with multiple units:
PT1H15M- Negative durations:
-PT1W1D2H3M4S- Different time units: hours (H), minutes (M), seconds (S), weeks (W), days (D)
The updated duration
PT1H15Mintest_date_time_duration.icscomplements the existing test cases and helps ensure proper handling of multi-unit durations.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for other duration formats in test files to ensure comprehensive testing # Expect: Various duration formats including multiple time units # Search for DURATION properties in all .ics files rg "DURATION:" -A 1 "*.ics"Length of output: 73
Script:
#!/bin/bash # Let's try searching with fd first to locate .ics files fd -e ics # Then search for DURATION in those files fd -e ics --exec rg "DURATION:" -A 1 {}Length of output: 1218
test/test-async.js (1)
854-854: LGTM! The test assertion has been correctly updated.The change properly reflects the expected end time for an event with a duration of 1 hour and 15 minutes, fixing the handling of multiple time units in duration calculations.
Let's verify that the corresponding .ics file has been updated with the correct duration:
✅ Verification successful
✓ Test assertion matches the ICS file duration
The test assertion expecting end time
2024-02-15T10:15:00.000Zcorrectly aligns with:
- Start time:
20240215T090000Z(9:00 AM UTC)- Duration:
PT1H15M(1 hour and 15 minutes)🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the duration in test_date_time_duration.ics matches the test assertion # Test: Search for DURATION property in the test file rg "DURATION:|DTSTART:" test/test_date_time_duration.icsLength of output: 100
test/test.js (1)
1044-1044: LGTM! The test case correctly verifies the duration calculation.The assertion validates that an event starting at 09:00 UTC with a duration of 1 hour and 15 minutes (PT1H15M) correctly ends at 10:15 UTC.
Let's verify that the test case matches the ICS file:
✅ Verification successful
Test case correctly matches the ICS file specification
The verification confirms that:
- The ICS file has an event starting at 09:00 UTC (
DTSTART:20240215T090000Z)- The duration is exactly 1 hour and 15 minutes (
DURATION:PT1H15M)- This validates that the test assertion checking for an end time of 10:15 UTC is correct, as it properly accounts for the duration
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the duration in the test_date_time_duration.ics file # Expected: Should find PT1H15M duration # Test: Search for DURATION property rg "DURATION:PT1H15M" test/test_date_time_duration.icsLength of output: 71
Script:
#!/bin/bash # Let's verify the complete event details in the ICS file # to ensure start time and other properties match the test case cat test/test_date_time_duration.icsLength of output: 448
ical.js
Outdated
| const duration = curr.duration.match(/-?\d{1,10}[YMWDHS]/g); | ||
|
|
||
| // Use the duration to create the end value, from the start | ||
| let newend = moment.utc(curr.start); | ||
| let newEnd = moment.utc(curr.start); | ||
| // Is the 1st character a negative sign? | ||
| const indicator = curr.duration.startsWith('-') ? -1 : 1; | ||
| newend = newend.add(Number.parseInt(r, 10) * indicator, durationUnits[r.toString().slice(-1)]); | ||
|
|
||
| for (const r of duration) { | ||
| newEnd = newEnd.add(Number.parseInt(r, 10) * indicator, durationUnits[r.toString().slice(-1)]); | ||
| } | ||
|
|
||
| // End is a Date type, not moment | ||
| curr.end = newend.toDate(); | ||
| curr.end = newEnd.toDate(); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove unused duration units and add validation.
The duration calculation has been improved to handle multiple time units correctly. However, there are a few suggestions for improvement:
- The
durationUnitsobject includes 'Y' (years) and 'M' (months) in the comment, but they're not used. This could be misleading. - There's no validation for invalid duration units.
Consider applying these improvements:
const durationUnits =
{
- // Y: 'years',
- // M: 'months',
W: 'weeks',
D: 'days',
H: 'hours',
M: 'minutes',
S: 'seconds'
};
const duration = curr.duration.match(/-?\d{1,10}[YMWDHS]/g);
+// Validate duration units
+for (const r of duration) {
+ const unit = r.slice(-1);
+ if (!durationUnits[unit]) {
+ throw new Error(`Invalid duration unit: ${unit}`);
+ }
+}
let newEnd = moment.utc(curr.start);Consider caching the moment object to improve performance when processing multiple duration components:
-let newEnd = moment.utc(curr.start);
+const startMoment = moment.utc(curr.start);
+let newEnd = startMoment;Committable suggestion was skipped due to low confidence.
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
ical.js (1)
486-486: Consider adding a comment explaining the date conversion.The conversion from moment to Date type is important but not immediately obvious.
Add a comment to improve code readability:
-curr.end = newEnd.toDate(); +// Convert from moment to Date type as required by the API +curr.end = newEnd.toDate();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
- ical.js (1 hunks)
- test/test-async.js (1 hunks)
- test/test.js (1 hunks)
- test/test_date_time_duration.ics (1 hunks)
🔇 Additional comments (6)
test/test_date_time_duration.ics (1)
16-16: LGTM! The duration format is valid and demonstrates the multiple time units use case.The updated duration
PT1H15Mfollows the ISO 8601 duration format and correctly represents a period of 1 hour and 15 minutes, which is suitable for testing the handling of multiple time units.Let's verify the duration format against the iCalendar spec:
✅ Verification successful
LGTM! The duration format is valid and correctly represents the test case.
The duration
PT1H15Min the iCalendar file follows the ISO 8601/RFC 5545 format and properly demonstrates the multiple time units use case (1 hour and 15 minutes) for testing the duration calculation fix.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the duration format follows RFC 5545 section 3.3.6 # Expected format: P[n]DT[n]H[n]M[n]S where [n] is a positive integer # Test: Search for any duration that doesn't match the RFC 5545 format rg -U 'DURATION:(?!P(\d+D)?(T(\d+H)?(\d+M)?(\d+S)?)?$)' test/test_date_time_duration.ics # No output expected, indicating the format is validLength of output: 350
Script:
#!/bin/bash # Let's try a simpler approach to verify the duration format # First, let's see the full content of the test file cat test/test_date_time_duration.ics # Then search for all DURATION lines to verify the format rg "DURATION:" test/test_date_time_duration.icsLength of output: 513
ical.js (2)
461-472: LGTM! Duration units are correctly defined.The duration units are properly defined according to RFC5545 3.3.6, correctly excluding year and month units.
475-477: 🛠️ Refactor suggestionConsider caching the moment object.
Creating a new moment object and reassigning it in each iteration could impact performance. The
startMomentis already created but not fully utilized.Consider this optimization:
-const startMoment = moment.utc(curr.start); -let newEnd = startMoment; +const newEnd = moment.utc(curr.start);Likely invalid or redundant comment.
test/test-async.js (1)
854-854: LGTM! The updated end time correctly reflects the duration.The change from
09:15to10:15aligns with the fix for handling multiple time units in duration (PT1H15M = 1 hour and 15 minutes).Let's verify the corresponding ICS file content:
✅ Verification successful
✅ Verified: The ICS file duration matches the expected end time
The ICS file contains
DURATION:PT1H15Mwhich correctly corresponds to the updated test assertion expecting an end time of 10:15 (start time 09:00 + duration of 1 hour and 15 minutes).🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify that the test_date_time_duration.ics file contains the correct DURATION property # Expected: Should find PT1H15M as the duration value rg "DURATION:PT1H15M" test/test_date_time_duration.icsLength of output: 71
test/test.js (2)
1044-1044: LGTM! The updated assertion correctly verifies the duration calculation.The modified assertion verifies that an event with a duration of 1 hour and 15 minutes (PT1H15M) correctly ends at 10:15:00 UTC when starting at 09:00:00 UTC. This change aligns with the PR's objective of fixing duration calculations with multiple time units.
1043-1044: Verify test coverage for other duration combinations.While this test case covers a duration with hours and minutes (PT1H15M), it would be beneficial to add more test cases covering other combinations of time units (e.g., days+hours, minutes+seconds) to ensure comprehensive coverage of the duration calculation fix.
Consider adding test cases for these scenarios:
- Multiple days + hours (e.g., P2DT5H)
- Hours + minutes + seconds (e.g., PT1H30M45S)
- Edge cases like single units (PT1H) vs multiple units (PT1H0M)
ical.js
Outdated
| for (const r of duration) { | ||
| newEnd = newEnd.add(Number.parseInt(r, 10) * indicator, durationUnits[r.toString().slice(-1)]); | ||
| } | ||
|
|
There was a problem hiding this comment.
Add validation for duration units.
The code assumes all duration units in the input are valid. Adding validation would make the code more robust.
Consider adding validation:
for (const r of duration) {
+ const unit = r.slice(-1);
+ if (!durationUnits[unit]) {
+ throw new Error(`Invalid duration unit: ${unit}`);
+ }
newEnd = newEnd.add(Number.parseInt(r, 10) * indicator, durationUnits[r.toString().slice(-1)]);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| for (const r of duration) { | |
| newEnd = newEnd.add(Number.parseInt(r, 10) * indicator, durationUnits[r.toString().slice(-1)]); | |
| } | |
| for (const r of duration) { | |
| const unit = r.slice(-1); | |
| if (!durationUnits[unit]) { | |
| throw new Error(`Invalid duration unit: ${unit}`); | |
| } | |
| newEnd = newEnd.add(Number.parseInt(r, 10) * indicator, durationUnits[r.toString().slice(-1)]); | |
| } |
Test speaks for itself I guess. When having multiple units in your duration, the end was not calculated correctly
Summary by CodeRabbit
New Features
Bug Fixes
Tests