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

Add the information that states that the datas were successfully parsed #337

Closed
wants to merge 2 commits into from

Conversation

axi
Copy link

@axi axi commented Apr 22, 2024

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.

@s0600204
Copy link
Collaborator

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.

@axi
Copy link
Author

axi commented Apr 24, 2024

@s0600204 this makes sense, I pushed an update

@u01jmg3 u01jmg3 self-requested a review April 24, 2024 20:11
@u01jmg3
Copy link
Owner

u01jmg3 commented Apr 30, 2024

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.

@axi
Copy link
Author

axi commented Apr 30, 2024

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.

@s0600204
Copy link
Collaborator

s0600204 commented May 2, 2024

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.

@u01jmg3
Copy link
Owner

u01jmg3 commented May 2, 2024

Thanks for your assessment, @s0600204.

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

@u01jmg3 u01jmg3 closed this May 2, 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