Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add .appendAll() function #35

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Conversation

KaidenR
Copy link

@KaidenR KaidenR commented Apr 8, 2017

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
Copy link
Author

KaidenR commented Apr 8, 2017

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

@codecov
Copy link

codecov bot commented Apr 10, 2017

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%> (ø) ⬆️

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
Copy link
Owner

Isn't this essentially

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

@KaidenR
Copy link
Author

KaidenR commented Apr 10, 2017

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
Copy link
Owner

felixfbecker commented Jun 28, 2017

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
Copy link

Ashoat commented Jan 17, 2018

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
Copy link

christiaanwesterbeek commented May 6, 2020

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
Copy link

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Helper for join array of SQLStatements?
5 participants