felixfbecker / node-sql-template-strings

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

Add .appendAll() function #35

Open KaidenR opened 7 years ago

KaidenR commented 7 years ago

Adds an .appendAll() function that allows you to pass an array of statements or strings to append. You can also provide a custom delimiter to separate the statements/strings (defaults to a single space)

Also updates Typescript typings, and README file with documentation. Includes several unit tests.

Closes #32

KaidenR commented 7 years ago

Another option here would be to include this functionality in the .append() function.

codecov[bot] commented 7 years ago

Codecov Report

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

@@          Coverage Diff          @@
##           master    #35   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files           1      1           
  Lines          28     32    +4     
=====================================
+ Hits           28     32    +4
Impacted Files Coverage Δ
index.js 100% <100%> (ø) :arrow_up:

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...c64c93c. Read the comment docs.

felixfbecker commented 7 years ago

Isn't this essentially

toAppend.reduce((prev, curr) => prev.append(delim).append(curr), stmt)
KaidenR commented 7 years ago

Yeah that does the same thing but it's more difficult to read and prevents you from being able to chain additional append()s. Also, the for-loop will be more performant than a map(). curr will often be pretty long and/or complicated so it would be nice to be able to have the surrounding syntax as terse as possible. That may be another reason for moving this into the append() function instead of creating a new one.

What do you think?

felixfbecker commented 7 years ago

Did you actually measure a performance difference? I doubt it has one (after all, you are IO-bound anyway for database queries). Readability is a matter of taste, if I read reduce(), I instantly know what it does, while appendAll is a custom method where I need to "guess" its meaning (albeit pretty obvious). I am reluctant to add helper methods (and the associated docs + tests) for features that can be solved functionally with vanilla JS... The biggest trait of this module is simplicity, and I don't want to bloat the API. Initially it didn't even have append, but that proved to be something that is non-trivial to do without a method.

Ashoat commented 6 years ago

Just wanted to chime in on this ancient PR. I agree shorthand functions can sometimes make readability worse, but in this case I think the pattern in question is so commonplace that the cost of having to read a new custom method and "guess" its meaning is less than the cost of having to parse this pattern out on every occurrence.

christiaanwesterbeek commented 4 years ago

Isn't this essentially

toAppend.reduce((prev, curr) => prev.append(delim).append(curr), stmt)

It seems to me that in most cases you want the reduce-version adding the delimiter only starting from the second one and beyond

toAppend.reduce((prev, curr, index) =>
 (index > 0 ? prev.append(delim) : prev).append(curr), stmt)
valoricDe commented 2 years ago

@felixfbecker Hi there. I don't know if everything has been said on this topic. But I agree on the stated positions that an appendAll would be helpful. https://www.npmjs.com/package/pg-template-tag solves this with a SQL.join function.