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

Applying modification pattern in #385 to FilteringParserDelegate#_nextToken2 #642

Closed
wants to merge 1 commit into from

Conversation

T45K
Copy link

@T45K T45K commented Oct 2, 2020

In #385, FilteringParserDelegate#nextToken was modified, but its similar method, _nextToken2 was not modified.
Isn't the same modification needed?

@cowtowncoder
Copy link
Member

cowtowncoder commented Oct 2, 2020

Possibly; I'd need to take a closer look.

But what would be really useful would be a test case to show that it is needed, and problem this would fix.
Not sure how easy that would be create; it's just that the whole handling is quite... delicate. So there is a possibility of handling changing in unintended way as testing is unfortunately not extensive for filtering.

@cowtowncoder
Copy link
Member

cowtowncoder commented Jul 20, 2021

Ended up making this change manually, as part of fix for #700. I think it is legit, although not sure how to reproduce specific problem.
Thank you @T45K for contributing it!

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