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

Handle case when SQLStatement passed as parameter. #31

Closed
wants to merge 2 commits into from

Conversation

Strate
Copy link

@Strate Strate commented Mar 12, 2017

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

codecov bot commented Mar 12, 2017

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

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

Strate commented Mar 24, 2017

Could you please give me some feedback on this?

@felixfbecker
Copy link
Owner

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

Strate commented Mar 24, 2017

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?

} else {
this.strings = []
this.values = []
let prevOpened = false
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this mean?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It means that previous value of this.strings array was created by append function, and next value of strings should be appended to previous value of this.strings, instead of creating new one

index.js Show resolved Hide resolved
@Strate
Copy link
Author

Strate commented Mar 30, 2017

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

@felixfbecker
Copy link
Owner

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.

@Strate Strate closed this Oct 11, 2017
@Strate Strate mentioned this pull request Nov 13, 2017
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.

2 participants