Skip to content

Conversation

@madjar
Copy link
Contributor

@madjar madjar commented Apr 30, 2025

I ran the parser on my google calendar export (after the fixes from #2), and I noticed that I have events with a DTSTART and no DTEND, which the parser doesn't like. This PR ensures that we can resolve the nil dtend to nil :)

I couldn't find place where a test would fit, but I'm happy to add one if you point me in the right direction.

@jonathanballs
Copy link
Owner

jonathanballs commented Apr 30, 2025

Hi @madjar,

Thanks for the PR - that's a good catch. I suggest throwing something like this at the bottom of test/magical/parser_test.exs. Then I'd be happy to merge :)

  describe "time zone resolution" do
    test "gracefully handles nil start or end times" do
      {:ok,
       %Magical.Calendar{
         events: [
           %Magical.Event{
             dtstart: ~D[2018-01-01],
             dtend: nil
           }
         ]
       }} =
        """
        BEGIN:VCALENDAR
        BEGIN:VEVENT
        DTSTART;VALUE=DATE:20180101
        END:VEVENT
        END:VCALENDAR
        """
        |> Parser.parse()
    end
  end

@jonathanballs
Copy link
Owner

I noticed that you have found a couple of extra edge cases that you mentionned in the other PR. I will review them shortly.

@madjar
Copy link
Contributor Author

madjar commented May 2, 2025

Updated with the test!

I tweak the one you provided a bit, since the test ics needs to have a timezone to enable that code path. I also went with a datetime to exercise the conversion behaviour.

@jonathanballs jonathanballs merged commit 9b6fb3c into jonathanballs:main May 2, 2025
2 checks passed
@jonathanballs
Copy link
Owner

Merged - thank you for your contribution! I will wait until the other PR for alarms is merged before pushing a new hex release 👍

@madjar
Copy link
Contributor Author

madjar commented May 2, 2025

Thanks!

@madjar madjar deleted the no-dtend branch August 20, 2025 16:38
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.

2 participants