ics-parser icon indicating copy to clipboard operation
ics-parser copied to clipboard

Add the information that states that the datas were successfully parsed

Open axi opened this issue 1 year ago • 4 comments

Similar to #336 but just validating that some content has been parsed and no error has been thrown. Without this, there's no way to distinguish the state of the parser before and after parsing happened. Another idea could be to set $cal property to null before parsing but this would be a BC.

axi avatar Apr 22 '24 23:04 axi

Without this, there's no way to distinguish the state of the parser before and after parsing happened.

As per one of your own comments on the previous iteration, with this there's still no way to distinguish between the state of the parser before and after parsing a second (or third, or fourth, or...) input file/string/url. Whilst setting $cal to null could indeed be considered a breaking change, setting your new variable to false at the start of initLines() would not.

s0600204 avatar Apr 24 '24 18:04 s0600204

@s0600204 this makes sense, I pushed an update

axi avatar Apr 24 '24 18:04 axi

I must admit I'm still not happy with merging this code. It does not indicate why content wasn't parsed which, if I was making use of this code, would be my next question. For me, I feel the implementation is too vague.

@s0600204: I'd be keen to hear your thoughts.

u01jmg3 avatar Apr 30 '24 11:04 u01jmg3

In my opinion, knowing that a stream/file/content is "invalid" in this context, even if I don't know why it's invalid, is more valuable that not having a way to get that information.

axi avatar Apr 30 '24 15:04 axi

I'd say it depends on the intended objective.

If the objective is to determine whether a provided file passes through the parser without error, then

$parsedSuccessfully = false;
try {
    $ical = new ICal('./example_ical.ics');
    $parsedSuccessfully = true;
} catch (Exception $except) {}

would probably work just as well.

If the objective is to determine if the passed file/string/url is valid ICal data, then this PR fails as - like its predecessor - it only really checks for the existence of BEGIN:VCALENDAR.

Just because it passes through our parser without error does not necessarily mean that it's valid ICal. The following currently pass through the parser without error:

BEGIN:VCALENDAR
BEGIN:VCALENDAR
BEGIN:EVENT
DTSTART:20240101

Are they "valid"? No. Are they successfully parsed? Yes.

s0600204 avatar May 02 '24 18:05 s0600204

Thanks for your assessment, @s0600204.

I agree and still don't think there's enough merit in merging this code.

u01jmg3 avatar May 02 '24 19:05 u01jmg3