-
-
Notifications
You must be signed in to change notification settings - Fork 10
Improve error message for missing operands #221
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
Conversation
ehuelsmann
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking good!
luke-hill
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This does look good. But I cannot trace why this custom pop method was ever used.
But given what you've written and the error examples I can clearly see that for each of them it would be almost impossible to work out "what" the problem is. i.e. for a and is the problem the and or the missing second item.
Overall it's a 👍 from me, but I can't quite trace / understand the logic
|
Question: in the addition to the errors test data, I notice that the tests being added are adding a new error message rather than reusing an existing one. The input Why the difference? |
Good point. Addressed by d2d19c5 |
I don't know either. I suspect it was an over abundance of caution. |
…ken ToString() should be empty string rather than "true").
🤔 What's changed?
Improved the error message for cases where an operand is missing.
Unfortunately the parsing algorithm can't detect which operator is
missing operands so the error message must remain fairly generic.
⚡️ What's your motivation?
Fixes: #196
🏷️ What kind of change is this?
📋 Checklist: