Add SplitRawStatements() #102
Conversation
separator_test.go
Outdated
| errRe *regexp.Regexp | ||
| want []string | ||
| }{ | ||
| {desc: "empty input", input: "", want: []string{""}}, |
There was a problem hiding this comment.
This spec seems inconsistent with other specs to me.
That is, SeparateRawStatements(";") returns []{""}, so I guess that SeparateRawStatements omits the last empty statement and SeparateRawStatements("") returns []{}, but this spec says it is NO.
At least, we should note this behavior in the doc.
There was a problem hiding this comment.
OK. I will write semantics to the go doc comment.
TL;DR
Every statements are terminated by ;, <eof>, or ;<eof> . So, empty input is interpreted as a single empty statement.
separator.go
Outdated
| // filepath can be used in error message. | ||
| // | ||
| // [terminating semicolons]: https://cloud.google.com/spanner/docs/reference/standard-sql/lexical#terminating_semicolons | ||
| func SeparateRawStatements(filepath, s string) ([]string, error) { |
There was a problem hiding this comment.
I think Split sounds better naming than Separate. Also, please use src instead of s because this is a public API.
There was a problem hiding this comment.
I think Split sounds better naming than Separate.
Would it be better to rename to splitter.go?
Also, please use src instead of s because this is a public API.
I believe s string is widely accepted in the Go community.
I prefer input over src because it's not a src and dest pattern.
There was a problem hiding this comment.
Would it be better to rename to splitter.go?
Yes, I think split.go is more preferable because this file does not define Splitter.
I believe
s stringis widely accepted in the Go community.
I preferinputoversrcbecause it's not asrcanddestpattern.
"go/parser".ParseFile use src. I know this type is any, and more specifically it is one of string, []byte, and io.Reader, but I think it is appropriate to name this argument src.
There was a problem hiding this comment.
Yes, I think
split.gois more preferable because this file does not define Splitter.
+1
"go/parser".ParseFileusesrc. I know this type isany, and more specifically it is one ofstring,[]byte, andio.Reader, but I think it is appropriate to name this argumentsrc.
I think it is because go/parser is package for Go source files.
Package parser implements a parser for Go source files.
I don’t feel that calling .sql files as SQL source file is majority, and this function can be used for non-file input like stdin or --sql option.
There was a problem hiding this comment.
It is major to call a string to be parsed as source in the parsing context, and I believe it is not different in SQL implementations. However, I have no strong opinion. Okay, I accept the argument s.
There was a problem hiding this comment.
s sounds better than input.
|
Thank you @apstndb! |
It is known that statement separating without parsing syntax is needed for Cloud Spanner related OSSs.
cloudspannerecosystem/wrench#83
cloudspannerecosystem/yo#116
memefish is good place for statement separator, because the statement separator must know about lexical structure like strings and comments
and cloudspannerecosystem is reliable org.
Test cases should be added as
Note: I think it is no need to support custom separator like spanner-cli's
\G.