CSNW / sql-bricks

Transparent, Schemaless SQL Generation
http://csnw.github.io/sql-bricks
MIT License
205 stars 25 forks source link

Add union #1

Closed ghost closed 10 years ago

ghost commented 10 years ago

add support for unions, in this format: select().from('table').union(.select().from())

The more I think about it, the more I think we should do this format: select().from('table').union().select().from('table')

review: @prust

RandyPearson commented 10 years ago

Ostensibly, I agree with @schuttsmcsnw's preferred format. Gets clearer when you extrapolate to 3+ members in the union: the former would require ever deepening nested parens, while the latter remains clean, and easier to edit.

prust commented 10 years ago

@schuttsmcsnw, @RandyPearson:

Ok, after thinking about it a bit, I agree that SS's preferred format is more readable and more guessable / transparent / consistent, but I would like to also retain support for the current API (stmt.union(stmt, stmt, ...)) as it allows for easier re-use and composition.

At first, I thought it would require deep changes to implement SS's preferred format, but then I realized that it's not hard at all -- instead of the normal chaining (returning this), we can return a new Statement() that is unioned to the first one, like this:

function union(stmt) {
  if (stmt) {
    this._union = stmt;
    return this;
  }
  else {
    this._union = new Statement('select');
    return this._union;
  }
}

What do you think?

ghost commented 10 years ago

@prust - I agree on the union, I'll give this a shot & see how it goes.

ghost commented 10 years ago

done - ready for another round of review: @prust