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

Correct closing of NODE_ITEM #280

Merged
merged 1 commit into from
Nov 6, 2024

Conversation

i-n-g-o
Copy link
Contributor

@i-n-g-o i-n-g-o commented Oct 17, 2024

When OnEndElement(...) is called ctxt.in_item is still > 0. Therefore checking if the closing node is NODE_ITEM needs to happen inside that conditional block to decrease ctxt.in_item and possibly stop parsing.

When OnEndElement(...) is called ctxt.in_item is still > 0.
Therefore checking if the closing node is NODE_ITEM needs to happen
inside that conditional block to decrease ctxt.in_item and possibly
stop parsing.
@vslavik
Copy link
Owner

vslavik commented Oct 23, 2024

I'm sorry, but even after several re-readings, I don't understand why you want to make the change. The commit message explains what the change is (something the code already answers), but not why.

Can you please explain what is handled incorrectly and provide an example feed content that demonstrates it? TIA!

@i-n-g-o
Copy link
Contributor Author

i-n-g-o commented Oct 24, 2024

As far as I can see the code to preliminary stop parsing can never be called. It seemed not to be intended. That is why I proposed this PR. Maybe I am missing something and the original code is perfectly fine - in that case this PR of course can be discarded.

@vslavik
Copy link
Owner

vslavik commented Nov 1, 2024

No, you're right, I just couldn't understand what you were saying, because that bug was so absurd it didn't register... Sorry for being slow!

@vslavik vslavik merged commit 4c74b89 into vslavik:master Nov 6, 2024
6 checks passed
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