-
Notifications
You must be signed in to change notification settings - Fork 140
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
Start development of a new pretty printer for Cadence programs #1024
Conversation
Codecov Report
@@ Coverage Diff @@
## master #1024 +/- ##
==========================================
- Coverage 77.31% 77.22% -0.09%
==========================================
Files 274 278 +4
Lines 35347 35814 +467
==========================================
+ Hits 27327 27659 +332
- Misses 6941 7069 +128
- Partials 1079 1086 +7
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
Cadence Benchstat comparisonThis branch with compared with the base branch onflow:master commit 0e7825d Results
|
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 awesome! 🤩
One general question I had was, would it make sense to move the formatting/prettying codes to a separate visitor?
"strings" | ||
|
||
"github.com/turbolent/prettier" |
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.
😍 🎉
@@ -34,6 +35,7 @@ type Expression interface { | |||
IfStatementTest | |||
isExpression() | |||
AcceptExp(ExpressionVisitor) Repr | |||
Doc() prettier.Doc |
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 guess this returns the 'textual' representation of the expression. Would it make sense to rename it to something that reflects it? People might confuse it with 'documentation/doc-comments' (happened to me: I was wondering for a moment, can there be any doc-comments for expressions 😄 )
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.
It didn't occur to me, bad that's a good point! Would PrettyPrintDocument()
be a better name? Any other suggestions? Naming is hard, haha
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.
Yea PrettyPrintDocument()
sounds a better option. Some other names I could think of are PrettyPrint()
, Format()
.
Alternatively, if we can move this to a separate visitor, then we wouldn't have this method anyway.
One general question I had was, would it make sense to move the formatting/prettying codes to a separate visitor?
That might be a big refactor though.
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.
Ah, I had missed that comment! Good idea, I might refactor the existing code to a separate visitor once the existing PRs are in
separatorDoc = memberExpressionSeparatorDoc | ||
} | ||
return prettier.Concat{ | ||
// TODO: potentially parenthesize |
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.
Probably a good solution is to introduce a ParenthesizedExpression
to the AST at the parser level, so we don't have to specially handle parentheses everywhere. Expression.Doc()
would delegate to ParenthesizedExpression.Doc()
which would then add the parentheses.
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.
of-course we don't have to do it here; just a suggestion for future improvements.
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.
That's an interesting idea! 👌 I'd love to understand it better, let's have a sync next week
prettier.Group{ | ||
Doc: leftDoc, | ||
}, | ||
prettier.Line{}, |
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.
is this going to be a newline always?
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.
prettier.Line
is rendered as ae space if there is space, or a newline if there is not.
The idea here is to break before the symbol, so that e.g. let a = 1 + 2 * 3
is rendered like this if it does not fit:
let a = 1
+ 2
* 3
@turbolent This is amazing. Is it any how feasible to port parser to javascript? Can be useful for fcl and js-testing probably. It is for a long time in my ideas list. If there is no obvious road blocks, I would like to work on this. |
@bluesign Parser is already available as an npm-package: https://github.com/onflow/cadence/tree/master/npm-packages/cadence-parser |
@bluesign Yeah, like @SupunS pointed out, it should be very little work to compile to expose this to e.g. JavaScript via WebAssembly, by adding functions to the parser NPM package. I don't know what the current status is for TinyGo, but it might be able to compile the parser to a smaller binary than the reference Go compiler. |
Work towards #209
Description
Start the development of a new pretty-printer for Cadence.
Implement pretty-printing functions for the following AST nodes:
ArrayExpression
BoolExpression
DestroyExpression
DictionaryExpression
ForceExpression
IdentifierExpression
IndexExpression
MemberExpression
NilExpression
PathExpression
StringExpression
UnaryExpression
InvocationExpression
IntegerExpression
CreateExpression
FixedPointExpression
ConditionalExpression
BinaryExpression
CastingExpression
ReferenceExpression
FunctionExpression
VariableDeclaration
ReturnStatement
Still missing for expressions:
String()
functions with pretty-print results (assume a default max line width)Also, implement a small web-based tool that helps with the development: It parses code into the AST and pretty-prints it, given a maximum line width.
master
branchFiles changed
in the Github PR explorer