Skip to content
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

ast: Add TypeDecl Node #5102

Merged
merged 1 commit into from
Apr 8, 2024
Merged

ast: Add TypeDecl Node #5102

merged 1 commit into from
Apr 8, 2024

Conversation

mattnibs
Copy link
Collaborator

@mattnibs mattnibs commented Apr 5, 2024

This commit adds a separate AST decl type for type declarations as opposed to using const deleclartions for this case. This is part of a series of changes preparing the AST code base to support position information for AST nodes.

@mattnibs mattnibs requested a review from a team April 5, 2024 21:43
@mattnibs mattnibs changed the title ast: A TypeDecl Node ast: Add TypeDecl Node Apr 5, 2024
compiler/ast/ast.go Outdated Show resolved Hide resolved
if err := a.scope.DefineConst(a.zctx, d.Name, e); err != nil {
return dag.Def{}, err
}
return dag.Def{Name: d.Name, Expr: e}, nil
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: These are on multiple lines everywhere else in here.

Suggested change
return dag.Def{Name: d.Name, Expr: e}, nil
return dag.Def{
Name: d.Name,
Expr: e
}, nil

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For me I follow my made up rule that if a struct declaration fits below 80 characters, I do it single line. I think that seems reasonable, no?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Totally reasonable. I just prefer to see changes follow the prevailing style. Just a nit though so do as you like!

zfmt/ast.go Outdated Show resolved Hide resolved
@mattnibs mattnibs force-pushed the type-decl branch 2 times, most recently from 3fa90aa to 8e97d69 Compare April 6, 2024 18:46
Base automatically changed from remove-pegjs to main April 8, 2024 16:33
This commit adds a separate AST decl type for type declarations as
opposed to using const deleclartions for this case. This is part of a
series of changes preparing the AST code base to support position
information for AST nodes.
Copy link
Member

@nwt nwt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So nice I'm approving it twice.

@mattnibs mattnibs merged commit 1bc062d into main Apr 8, 2024
3 checks passed
@mattnibs mattnibs deleted the type-decl branch April 8, 2024 16:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants