Skip to content
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

recur_expansion: EXDATE can be DATE and DTSTART can be DATE-TIME #670

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dilyanpalauzov
Copy link
Contributor

When EXDATE is in DATE format and DTSTART is DATE-TIME, then to determine whether an occurrence shall be excluded, the occurrence shall be converted to DATE and then compared to EXDATE.

This applies also for the very first EXDATE, when it coincides with DTSTART.

I had this change locally since 2022. I have not uploaded it, as I have lost hope that PRs here will be handled.

@coveralls
Copy link

coveralls commented Apr 21, 2024

Pull Request Test Coverage Report for Build 9821264905

Details

  • 21 of 21 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.004%) to 98.164%

Totals Coverage Status
Change from base Build 9719236284: 0.004%
Covered Lines: 9472
Relevant Lines: 9634

💛 - Coveralls

Copy link
Owner

@kewisch kewisch left a comment

Choose a reason for hiding this comment

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

Thanks for the patch! I'm going to need a few tests for this one, would appreciate if you could add them.

Comment on lines +211 to +212
if (!a.isDate && b.isDate)
return new Time({ year: a.year, month: a.month, day: a.day }).compare(b);
Copy link
Owner

Choose a reason for hiding this comment

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

This also strips out the timezone, which might affect the comparison. Can you add zone handling here?

Copy link
Owner

Choose a reason for hiding this comment

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

A test or two around this case and zone handling would also be useful.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have added a test.

I am not going to add time zone handling, as I do not understand how it would work. This patch fixes a problem I faced in 2022. I do not work with iCalendar in JavaScript currently.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This also strips out the timezone, which might affect the comparison. Can you add zone handling here?

The commit message contains:

When EXDATE is in DATE format and DTSTART is DATE-TIME, then to determine whether an occurrence shall be excluded, the occurrence shall be converted to DATE and then compared to EXDATE.

I would say in the conversion of VALUE=DATE-TIME to VALUE=DATE the timezone is irrelevant.

@kewisch
Copy link
Owner

kewisch commented Jun 5, 2024

@dilyanpalauzov Did you have a chance to look into this? Would appreciate if you could take care of the review comments.

@kewisch kewisch added the needinfo More information has been requested label Jun 5, 2024
@dilyanpalauzov dilyanpalauzov force-pushed the recur_expansion_exdate branch 2 times, most recently from 1722292 to 184e9e9 Compare June 9, 2024 06:40
When EXDATE is in DATE format and DTSTART is DATE-TIME, then
to determine whether an occurrence shall be excluded, the
occurrence shall be converted to DATE and then compared to EXDATE.

This applies also for the very first EXDATE, when it coincides with
DTSTART.
Copy link
Contributor Author

@dilyanpalauzov dilyanpalauzov left a comment

Choose a reason for hiding this comment

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

I do not know how these reviews of my own change works. Apparently I have to add some comment for the sake of having any comment

Copy link

github-actions bot commented Jul 6, 2024

It looks like we haven't heard back on this issue, therefore we are closing this issue. If this problem persists in the latest version of ical.js, please re-open this issue.

@github-actions github-actions bot closed this Jul 6, 2024
@github-actions github-actions bot removed the needinfo More information has been requested label Jul 6, 2024
@dilyanpalauzov
Copy link
Contributor Author

?!?!

@kewisch kewisch reopened this Jul 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants