Skip to content

Fixed duration issue when having multiple time units #350

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Oct 28, 2024
Merged

Fixed duration issue when having multiple time units #350

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b66b57b
Fixed duration issue when having multiple time units
wouterlemcke Oct 28, 2024
2b90200
apply coderabbitai suggestions.
jens-maus Oct 28, 2024
6bdf1a3
apply coderabbitat suggestion to check duration units
jens-maus Oct 28, 2024
073da3e
style fix
jens-maus Oct 28, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 8 additions & 4 deletions ical.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -470,15 +470,19 @@ module.exports = {
S: 'seconds'
};
// Get the list of duration elements
const r = curr.duration.match(/-?\d{1,10}[YMWDHS]/g);
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)]);
}

Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue

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.

Suggested change
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)]);
}

// End is a Date type, not moment
curr.end = newend.toDate();
curr.end = newEnd.toDate();
Copy link

Choose a reason for hiding this comment

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

🛠️ Refactor suggestion

⚠️ Potential issue

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:

  1. The durationUnits object includes 'Y' (years) and 'M' (months) in the comment, but they're not used. This could be misleading.
  2. 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.

}
}

Expand Down
2 changes: 1 addition & 1 deletion test/test-async.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -851,7 +851,7 @@ vows
},
'it uses the start/end of the event'(event) {
assert.equal(event.start.toJSON(), '2024-02-15T09:00:00.000Z');
assert.equal(event.end.toJSON(), '2024-02-15T09:15:00.000Z');
assert.equal(event.end.toJSON(), '2024-02-15T10:15:00.000Z');
}
}
},
Expand Down
2 changes: 1 addition & 1 deletion test/test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1041,7 +1041,7 @@ vows
},
'it uses the start/end of the event'(event) {
assert.equal(event.start.toJSON(), '2024-02-15T09:00:00.000Z');
assert.equal(event.end.toJSON(), '2024-02-15T09:15:00.000Z');
assert.equal(event.end.toJSON(), '2024-02-15T10:15:00.000Z');
}
}
},
Expand Down
2 changes: 1 addition & 1 deletion test/test_date_time_duration.ics
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,6 @@ DESCRIPTION:
LOCATION:Kriftel
X-APPLE-STRUCTURED-LOCATION;VALUE=URI;X-TITLE=Kriftel: geo:50.083558\,8.4693855
DTSTART:20240215T090000Z
DURATION:PT15M
DURATION:PT1H15M
END:VEVENT
END:VCALENDAR
Loading