-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
Editorial: more explicit validation in CreateDynamicFunction #2374
Conversation
I am not a reviewer, but FWIW, LGTM. Thanks much! |
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.
I understand where @erights is coming from in #2373, but I find this piecemeal parsing, then re-parsing the entire text only for the Early Error application to be misleading. It's misleading in that it suggests that parsing expr using exprSym alone is somehow insufficient to do all the validation while it in fact is.
I'd prefer to keep to a single parse and have a NOTE that says the syntactic constraint that @erights wants highlighted is applied post-concatenation. Then again, I don't find that state of affairs particularly surprising or indirect as he does.
@bakkot Nice example! I tried to come up with one but did not. Thanks! |
Ah ha, I retract my above comment then. |
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.
lgtm with comment
I like it. But your example is worth a thousand such explanations since most of us didn't see how the original text was erroneous even after we started looking for a counter-example. Your example is tiny and clarifying. It would be a good addition. |
Added my above example as another NOTE. |
c263627
to
c754979
Compare
Fixes #2373. cc @erights