felixfbecker / node-sql-template-strings

ES6 tagged template strings for prepared SQL statements 📋
ISC License
610 stars 40 forks source link

Handle case when SQLStatement passed as parameter. #31

Closed Strate closed 7 years ago

Strate commented 7 years ago

This PR introduces ability to pass SQLStatement as parameter to other SQLStatement cosntructor. It could be used as more straighforward alternative to .append method.

See #30.

codecov[bot] commented 7 years ago

Codecov Report

Merging #31 into master will not change coverage. The diff coverage is 100%.

@@          Coverage Diff          @@
##           master    #31   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           1      1           
  Lines          28     47   +19     
=====================================
+ Hits           28     47   +19
Impacted Files Coverage Δ
index.js 100% <100%> (ø) :white_check_mark:

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update 7d38214...73d4280. Read the comment docs.

Strate commented 7 years ago

Could you please give me some feedback on this?

felixfbecker commented 7 years ago

This makes the code a whole lot more complicated and I don't understand what's going on in the constructor. It is also essentially the same semantic as append(), but doesn't share any of the code with it.

I do see the value for subqueries though, it is a lot nicer than append() for that use case.

Strate commented 7 years ago

I don't understand what's going on in the constructor

Actually, it transforms nested SQLStatements into chain of .append calls.

I do see the value for subqueries though, it is a lot nicer than append() for that use case.

Yes, subqueries is the main use-case for that. I think that this case is nicer than .append for almost any case :)

What should I do to get this merged? Simplify code? (I am really don't know how). Or something else?

Strate commented 7 years ago

Sorry for disturbing, but I want to ping this again up. Can I do something else to get this merged?

felixfbecker commented 7 years ago

Just took another look at it. What puts me off about this is that for "only" moving the capability to merge SQLStatements from append to the constructor, it makes the code a lot more complex and really hard to understand. append is just a few lines (and unchanged in this PR), but the constructor has a very complex loop now with a plethora of branches. My gut is telling me that there must be a way to do this with less code, maybe functional instead of iterative. And/or sharing code between append and the constructor.