Conversation
builder/query_test.go
Outdated
| }, | ||
| { | ||
| result: "SELECT `c`.`id`,`c`.`name` FROM `contacts` `c`;", | ||
| query: rel.Select("c.id").Select("c.name").From("contacts c"), |
There was a problem hiding this comment.
I wonder why not using single Select function? seems this change is also not related to the alias fix?
There was a problem hiding this comment.
I agree that is not related to aliases things, but I was thinking it would be beneficial and more convenient for everyone if we bring this small improvement as well.
My issue was to include some extra columns based on the condition, and it was more intuitive. I thought something else was broken, not my code when I was using it:
var query = rel.Select("id", "title").From("pools")
if includeAvgScore {
query.Select("avgScore")
} else {
query.Select("score")
}There was a problem hiding this comment.
Please feel free to make any changes. That was my opinion.
There was a problem hiding this comment.
I see, then can you create separate PR for this?
There was a problem hiding this comment.
Ok, Now we have alias changes only.
refactor escape func and fix failed tests
Trim spaces just to be more consistent
builder/buffer.go
Outdated
| b.WriteString(b.escape("", value)) | ||
| } | ||
|
|
||
| func (b Buffer) escape_schema(table string) string { |
builder/buffer.go
Outdated
| } | ||
| escaped_table = strings.Join(parts, ".") | ||
| escaped_table = b.escape_schema(table[:i]) + " AS " + b.Quoter.ID(strings.TrimSpace(table[i+4:])) | ||
| } else if i := strings.Index(strings.ToLower(table), " "); i > -1 { |
There was a problem hiding this comment.
this will break if the table name contains spaces
There was a problem hiding this comment.
What do you mean "table name contains spaces"? Is it possible?
I just wanted to avoid any issues if someone adds extra spaces like From("books b") or From("books as b").
See TrimSpace in b.Quoter.ID(strings.TrimSpace(table[i+4:])).
There was a problem hiding this comment.
Ok, I think I know what you are talking about.
} else if i := strings.Index(strings.ToLower(table), " "); i > -1 {
So you think someone could have something like:
.From("my books").From("my books b")// wherebis an alias.From("my books AS b").From("[my books] b")
I have never saw any examples where table name contains spaces in my lifetime. And I am not sure if such would properly work in any ORM system.
But it is a good point and I am not quite sure how we can identify it. I mean the examples 1 and 2 have no indication whether it is an alias or a table name. It could be either this [my] AS [books] or this [my books]
There was a problem hiding this comment.
Or you think we should remove that line and force everyone to use AS? So the syntax .From("books b") would not be valid.
There was a problem hiding this comment.
But based on probability people would want to write non space table queries more often rather then those who use spaces. The example with spaces is just some joke. You might as someone if they could define a variable with space:
var my book = rel.From("books")This is just my opinion. Once we allow to have spaces in table names, we are doomed.
There was a problem hiding this comment.
Who would convert the table name into the variable? Struct? Sorry we must keep some consistency.
It smells like fire to me.
You are talking about something which does not exist, and it shout not exists in back end world.
Please think about the rules of the backend, it must be strong and consistent.
Let me know if I need to change anything in the code.
There was a problem hiding this comment.
Are we forcing people write AS everywhere because of space in the table name?
There was a problem hiding this comment.
ok, please have a look at my last commit.
There was a problem hiding this comment.
I'm not saying we should force user to use AS, but we need to find a good way if we want to support that
As for now, I think bug in example 2 is more critical
2 participants
Closes #69