Skip to content

Comments

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

Merged
makenowjust merged 13 commits intocloudspannerecosystem:mainfrom
apstndb:feature/add-sql-pos-helper-functions
Oct 15, 2024
Merged

Add helper functions for SQL(), Pos(), End()#120
makenowjust merged 13 commits intocloudspannerecosystem:mainfrom
apstndb:feature/add-sql-pos-helper-functions

Conversation

@apstndb
Copy link
Contributor

@apstndb apstndb commented Sep 29, 2024

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.

Copy link
Collaborator

@makenowjust makenowjust left a comment

Choose a reason for hiding this comment

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

I have a comment to lastElem. Other functions are acceptable.

ast/util.go Outdated

// Helper functions for Pos(), End()

// lastElem returns last element of slice s.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I understand your intention, but this function seems too generic. I would not want to add this as it could be used in places it is not supposed to be.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK, lastElem can be written as inline, so I will remove it.

(It may be possible to be accepted if T is restricted to Node)

-func lastElem[T any](s []T) T {
+func lastElem[T Node](s []T) T {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed e1a3e81

Copy link
Collaborator

Choose a reason for hiding this comment

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

func lastElemNode[T Node](s []T) T { is acceptable.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Introduce func lastNode[T Node](s []T) T in 47ea709

@makenowjust
Copy link
Collaborator

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

@apstndb
Copy link
Contributor Author

apstndb commented Oct 15, 2024

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
Copy link
Contributor Author

apstndb commented Oct 15, 2024

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
Copy link
Collaborator

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

This sounds good 👍

@apstndb apstndb requested a review from makenowjust October 15, 2024 07:35
Copy link
Collaborator

@makenowjust makenowjust left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you!

@makenowjust makenowjust merged commit e57746c into cloudspannerecosystem:main Oct 15, 2024
@apstndb apstndb deleted the feature/add-sql-pos-helper-functions branch October 15, 2024 09:57
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