Add native golang formatter #388
Conversation
|
Ok @sbarzowski this is clearly too big to review thoroughly but maybe you want to look at ast.go, the lexer and parser + the refactoring of cmd/jsonnet ? Maybe we want to make the linter also use that utility library? |
|
Sounds good. A common set of |
cmd/jsonnetfmt/lib/fix_newlines.go
Outdated
| @@ -0,0 +1,305 @@ | |||
| /* | |||
There was a problem hiding this comment.
Putting this stuff under cmd/ is not ideal. I think this package should stay a thin wrapper. What about moving cmd/jsonnet/lib to internal/formatter?
pass/pass.go
Outdated
| limitations under the License. | ||
| */ | ||
|
|
||
| package pass |
There was a problem hiding this comment.
I like that it's a separate package, but I think it should start as internal.
pass/pass.go
Outdated
| type Context interface{} | ||
|
|
||
| // CompilerPass is an interface for a pass that transforms the AST in some way. | ||
| type CompilerPass interface { |
There was a problem hiding this comment.
I don't like the word "compiler" here. There is no compiler :-). Maybe ASTPass?
cmd/jsonnetfmt/lib/fix_newlines.go
Outdated
| // }] | ||
| // The outer array can stay unexpanded, because there are no newlines between | ||
| // the square brackets and the braces. | ||
| type FixNewlines struct { |
There was a problem hiding this comment.
Why export these passes if they're in the same package as Format? I think we should either:
a) Unexport these passes.
a) Extract each pass to its own internal-to-formatter package. They don't seem to depend (much) on jsonnetfmt.go, so perhaps that could work. I don't know if splitting stuff into tiny packages like that is actually a good idea.
There was a problem hiding this comment.
Now they're in internal, seems like this is OK.
cmd/utils.go
Outdated
| limitations under the License. | ||
| */ | ||
|
|
||
| package cmd |
There was a problem hiding this comment.
It's nice to have these utils extracted, but I would make it internal. Maybe cmd/internal/cmd/utils.go.
cmd/utils.go
Outdated
|
|
||
| // HandleMemProfile creates a memory profile if requested by environment | ||
| // variable. | ||
| func HandleMemProfile() { |
There was a problem hiding this comment.
Nit: Maybe CreateMemProfile, StartMemProfile or just MemProfile/ProfileMemory? Handle is almost meaningless.
| // This may emit a keyword or an identifier. | ||
| func (l *lexer) lexIdentifier() { | ||
| r := l.peek() | ||
| if !isIdentifierFirst(r) { |
There was a problem hiding this comment.
Nit: isIdentifier/isIdentifierFirst names are a bit confusing. I know they were there before this change, but perhaps we can rename them to something like isValidIdentifierChar / isValidIdentifierFirstChar? WDYT?
ast/ast.go
Outdated
| @@ -43,6 +43,9 @@ type Node interface { | |||
| FreeVariables() Identifiers | |||
| SetFreeVariables(Identifiers) | |||
| SetContext(Context) | |||
| OpenFodderPtr() *Fodder | |||
There was a problem hiding this comment.
This API with 3 functions is weird. Especially since modifying the fodder returned by OpenFodder is not safe - it still refers to the same memory chunk and the modification will be visible in the node.
I would have just one function OpenFodder here, which returns a pointer (like OpenFodderPtr currently).
(FYI I would also change FreeVariables and Context to just return a pointer if I was designing this interface again, but probably not worth it to break the API).
There was a problem hiding this comment.
I would also add a comment explaining that it's a fodder going before the node and it makes sense to have it as part of the common Node interface, because you can put a comment before any expression.
| @@ -1353,27 +1355,28 @@ func (p *parser) parse(prec precedence) (ast.Node, errors.StaticError) { | |||
| // --------------------------------------------------------------------------- | |||
|
|
|||
| // Parse parses a slice of tokens into a parse tree. | |||
| func Parse(t Tokens) (ast.Node, errors.StaticError) { | |||
| func Parse(t Tokens) (ast.Node, ast.Fodder, errors.StaticError) { | |||
There was a problem hiding this comment.
Let's mention in this function's comment that it returns a trailing fodder going after the returned node.
Or maybe we should create a struct for the node and trailingFodder? It's kinda ugly to return them separately here.
|
I'm done with the review @sparkprime. Great stuff, thanks a lot for getting this done :-). |
|
This is so awesome!! Any way to use it from go directly? Would love to ship it as part of Tanka |
|
Yes we should probably provide a public API for the top level functionality. Take a look at |
No description provided.