- Notifications
You must be signed in to change notification settings - Fork471
Skip trailing comma in explicit partial application#6949
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Skip trailing comma in explicit partial application#6949
Uh oh!
There was an error while loading.Please reload this page.
Conversation
9aa6b1a tob8428cfCompare| in | ||
| Doc.concat [Doc.lparen; arg_doc;Doc.rparen] | ||
| |args -> | ||
| (* Avoid printing trailing comma when there is ... in function application*) |
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 don't think it matters whether to check if... happens at the end because that's already considered as syntax error.
I think this looks good.@cristianoc any comments? |
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.
Would you add a syntax test case too, so we see it being exercised.
(FTR, those would fail before your failed test, so you can test them locally on any machine)
jscomp/syntax/src/res_printer.ml Outdated
| Doc.concat [Doc.lparen; arg_doc;Doc.rparen] | ||
| |args -> | ||
| (* Avoid printing trailing comma when there is ... in function application*) | ||
| let hasDotDotDot, printed_args= |
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.
would you do a little search if this functionality already exists somewhere else?
I kind of don't even remember using... verbatim as label, instead of some other attribute.
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 is what I see in the parser let attrs = if is_partial then [res_partial_attr] else [] in
so I'm not sure your code runs? In any case, adding a test will answer this.
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 wonder which is better, pattern match on the label or the expression like so,
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.
The label does not exist. In the ast.
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.
Think of it as app1(f, args) vs app2(f, args)
One being total and the other partial.
Internally the distinction is only the presence of the attribute.
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.
So "..." only exists as string in the source, but that's not how it's represented in the AST.
| if partialthen | ||
| let dummy=Ast_helper.Exp.constant (Ast_helper.Const.int0)in | ||
| let loc= | ||
| {Asttypes.txt="res.partial";Asttypes.loc= expr.pexp_loc} |
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.
For the dummy attribute, I addedres.partial so it can be pattern matched with the attribute rather than the label.
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 at previous code, it doesn't look like the choice of theexpression here matters. It only cares about the label, so tagging partial attribute here allows us to pattern match on the attribute rather than the label.
@shulhi Could you add a CHANGELOG entry? |
Done. Should we also backport this to v11? |
Yes, I think so. I will do cherry picking to v11 in bulk at a later time. |
* Check for dotdotdot in args* Check on attribute rather than label* Add tests* Fix naming convention* Update CHANGELOG
* Check for dotdotdot in args* Check on attribute rather than label* Add tests* Fix naming convention* Update CHANGELOG
* Check for dotdotdot in args* Check on attribute rather than label* Add tests* Fix naming convention* Update CHANGELOG
Fix#6948