cloudspannerecosystem / memefish

memefish is the foundation to analyze Spanner SQL
https://cloudspannerecosystem.dev/memefish/
MIT License
76 stars 19 forks source link

Add helper functions for SQL(), Pos(), End() #120

Closed apstndb closed 1 month ago

apstndb commented 1 month ago

Contributors need to maintain comments in ast/ast.go and implementations of SQL(), Pos(), and End() methods in ast/sql.go and ast/pos.go. I believe productability is improved if comments and implementations can be written as similar structure.

I propose to add helper functions to implement the methods. I believe these functions are useful as long as memefish have ast.Node interface .

Note These helper functions are already used in PRs(#99, #101, #111). sqlOpt needs comparable after Go 1.20, so this PR bump Go version to 1.20. I have refacotored ast.Select, ast.Path and ast.CallExpr to demonstrate the usage and effect of helper functions.

makenowjust commented 1 month ago

@apstndb If these PRs are merged, do you rewrite existing implementations with them as well?

apstndb commented 1 month ago

If these PRs are merged, do you rewrite existing implementations with them as well?

If you want to unify style of existing implementation immediately, I can rewrite them. If not, we will rewrite them as necessary, like the Boy Scout rule.

apstndb commented 1 month ago

I may be better to place functions into sql.go and pos.go rather than util.go. These functions won't be used in other files, and these files should be self-contained as possible.

makenowjust commented 1 month ago

I may be better to place functions into sql.go and pos.go rather than util.go.

This sounds good 👍