From 727009ed7a05aa7ca151d7d5df32ae63e976e0ae Mon Sep 17 00:00:00 2001 From: Zach Musgrave Date: Thu, 18 Jan 2024 14:29:28 -0800 Subject: [PATCH 1/2] Updated contribution guide --- CONTRIBUTING.md | 13 +++++++++---- server/ast/expr.go | 2 ++ 2 files changed, 11 insertions(+), 4 deletions(-) diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 43d931a3c6..6577ac3581 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -191,10 +191,15 @@ func assignTableDef(node tree.TableDef, target *vitess.DDL) error `tree.TableDef` is not a node that can be transformed into a [Vitess](https://github.com/dolthub/vitess) node. Instead, it's purpose is to modify other nodes, which in this case is `*vitess.DDL`. -If there are any functions that do not fall into these two categories, and that function belongs in the `ast` package, then create an issue/PR and we will discuss it there. +#### Rule 3: Functions modifying vitess AST nodes start with `translate` + +Functions that modify a vitess expression, e.g. to perform additional or common translation logic +after other translation steps, should start with `translate`. + +If there are any functions that do not fall into the categories above, and that function belongs in the `ast` package, then create an issue/PR and we will discuss it there. If we add another function type, then this guide will also be updated. -#### Rule 3: Each node gets its own file +#### Rule 4: Each node gets its own file With very few exceptions, each node should have its own file. While this will create many files, it also allows a developer to quickly find the file containing the functionality that they're interested in. @@ -204,7 +209,7 @@ All `tree.CompositeDatum` nodes are also `tree.Expr` nodes, and thus `nodeCompos In this case, `nodeCompositeDatum` is in [`expr.go`](https://github.com/dolthub/doltgresql/blob/main/server/ast/expr.go), since its implementation is essentially in [`expr.go`](https://github.com/dolthub/doltgresql/blob/main/server/ast/expr.go). This is technically a discrepancy though, and all such exceptions should be moved to their own files if they cause any confusion. -#### Rule 4: Node slices get their own functions +#### Rule 5: Node slices get their own functions Many nodes also have slice forms. For example `tree.Expr` has `tree.Exprs`, which is defined as `type Exprs []Expr`. @@ -213,7 +218,7 @@ It may seem unnecessary, but it follows in the spirit of [Rule 1](#rule-1-functi In these cases, we generally do not create a new file, which technically violates [Rule 3](#rule-3-each-node-gets-its-own-file). However, since most of these functions amount to mere convenience functions, we include them in the same file to reduce the total number of files. -#### Rule 5: All fields are accounted for +#### Rule 6: All fields are accounted for All functions must account for all fields, implementations, etc. of their respective inputs. All interfaces must handle all integrators through a `switch` statement. diff --git a/server/ast/expr.go b/server/ast/expr.go index e6f72a9d1a..d1a01176fd 100644 --- a/server/ast/expr.go +++ b/server/ast/expr.go @@ -579,6 +579,8 @@ func nodeExpr(node tree.Expr) (vitess.Expr, error) { } } +// translateConvertType translates the *vitess.ConvertType expression given to a new one, substituting type names as +// appropriate. An error is returned if the type named cannot be supported. func translateConvertType(convertType *vitess.ConvertType) (*vitess.ConvertType, error) { switch strings.ToLower(convertType.Type) { // passthrough types that need no conversion From d9b9a11357cc917fe2cb824b0f465a7c7fb774a3 Mon Sep 17 00:00:00 2001 From: Zach Musgrave Date: Mon, 22 Jan 2024 17:34:45 -0800 Subject: [PATCH 2/2] formatting --- server/ast/expr.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/server/ast/expr.go b/server/ast/expr.go index d1a01176fd..8bf9485c12 100644 --- a/server/ast/expr.go +++ b/server/ast/expr.go @@ -579,7 +579,7 @@ func nodeExpr(node tree.Expr) (vitess.Expr, error) { } } -// translateConvertType translates the *vitess.ConvertType expression given to a new one, substituting type names as +// translateConvertType translates the *vitess.ConvertType expression given to a new one, substituting type names as // appropriate. An error is returned if the type named cannot be supported. func translateConvertType(convertType *vitess.ConvertType) (*vitess.ConvertType, error) { switch strings.ToLower(convertType.Type) {