Skip to content

Add test case where parse, tokenizer fails with empty option #1131

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

agirton
Copy link

@agirton agirton commented Nov 2, 2018

I don't have a fix just yet, trying to understand the code first, but wanted to open this PR for any guidance on the correct place to fix this test case. My application is interacting with a service that has an empty option therefore the parser fails. I figured his library should fail gracefully if the user has not set any option.

It seems the best place to fix this would be in the parse.js file in the parseOption > parseOptionValue on line 561 and 564.

@alexander-fenster
Copy link
Contributor

Hi @agirton,

This looks like a bug: the parseOption function (in src/parse.js:533) expects an option value inside. This needs to be fixed in the option parse logic.

The test you suggested is actually incorrect because the // Some TODO comment will eat all the remaining characters of the string. If you remove this comment and just leave the option empty, it will be a valid test for this bug.

agirton added 2 commits June 27, 2019 00:18
If we hit a option value with no body, we need to skip
@agirton
Copy link
Author

agirton commented Jun 27, 2019

@alexander-fenster I've updated test and I believe I found a fix, let me know if this looks ok to you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants