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

Allow chaining of increment, decrement, and update #2740

Merged
merged 4 commits into from Oct 6, 2018

Conversation

Projects
None yet
4 participants
@wubzz
Copy link
Collaborator

wubzz commented Jul 31, 2018

Relates to #269

Changes include:

  • Chainable .increment, .decrement, .update in the same build-chain
  • Can call .increment / .decrement on same column multiple times
  • Uses reduce for final query value
  • Object syntax for .increment / .decrement
  • Generates queries with "column" + ? only Reverted!
  • Added .clearCounters() to clear previous .increment / .decrement calls in a build chain
  • If .increment / .decrement is called for a column that has already been specified in .update then value specified in .update will take presedence.

See tests for examples.

May have missed something or completely misinterpreted the issue, in which case please let me know.

wubzz added some commits Jul 31, 2018

//Skip?
if (has(data, column)) {
//Needed?
this.client.logger.warn(

This comment has been minimized.

@Zolmeister

Zolmeister Jul 31, 2018

Most warns have to do with feature support by different SQL engines;
whereas errors are generally thrown for incorrect query construction. I'd say throw an error.
(just glancing at source/tests)

This comment has been minimized.

@wubzz

wubzz Aug 1, 2018

Collaborator

I think its bold to assume calling .increments first and then .update with the same column to be a user mistake, it may just as likely be perfectly intentional.

No strong opinion on what should happen here, but definitely some form of message must be spit out of knex instead of silently ignoring. I opted for warn over error due to the argument above, but not 100% against changing this.

},

// Clears increments/decrements
clearCounters() {

This comment has been minimized.

@Zolmeister

Zolmeister Jul 31, 2018

Could you explain what this endpoint might be used for?
My reaction is that it feels unnecessary

This comment has been minimized.

@wubzz

wubzz Aug 1, 2018

Collaborator

It follows the same principals as .clearSelect(), .clearWhere(), .clearOrder().

I've yet to find a real life use-case for these functions myself, but for consistency I added one here as well. Suppose someone has found a use for the other clear-functions, else they wouldn't exist.

This comment has been minimized.

@capaj

capaj Dec 12, 2018

Contributor

@wubzz
we use it like this with objection.js for example:

image

@tgriesser

This comment has been minimized.

Copy link
Owner

tgriesser commented Oct 5, 2018

Can call .increment / .decrement on same column multiple times

I think this might be confusing behavior with the rest of the library, where latter calls overwrite the former.

@tgriesser

This comment has been minimized.

Copy link
Owner

tgriesser commented Oct 5, 2018

Otherwise I think it looks good, could you add a test for having multiple different columns called with increment/decrement.

@tgriesser
Copy link
Owner

tgriesser left a comment

See comments.

@wubzz

This comment has been minimized.

Copy link
Collaborator

wubzz commented Oct 5, 2018

@tgriesser I don't think its more confusing than

knex('test').insert({test: '123'}).insert({test2: '342'}).toString();
"insert into `test` (`test2`) values ('342')"

and

knex('test').update({test: '123'}).update({test: '321'}).toString();
"update `test` set `test` = '321'"

But its your call.

The original issue relates to updating and incrementing at the same time, which before this PR is not supported. But there are other ways of solving this too, such as

knex(table)
.update({
    column1: 'newvalue',
    column2: knex.increment(1),
});

This PR is a mere suggestion.

@tgriesser

This comment has been minimized.

Copy link
Owner

tgriesser commented Oct 5, 2018

@wubzz no sorry I just meant the idea of having

.increments('test', 10).increments('test', 20)

output

test = test + 30

rather than

test = test + 20

is confusing based on the api patterns elsewhere. Does that make sense?

@wubzz

This comment has been minimized.

Copy link
Collaborator

wubzz commented Oct 5, 2018

@tgriesser Yes, in that sense I can agree it's a bit weird. Although could be useful if its called using dynamic fields in a loop containing logic..

No opinion on this, I can change it if you want?

@tgriesser

This comment has been minimized.

Copy link
Owner

tgriesser commented Oct 5, 2018

Yeah let's change it. If people really need that logic they can just precompute the value in the loop and then set once with the final value value rather than having the api handle it.

@wubzz

This comment has been minimized.

Copy link
Collaborator

wubzz commented Oct 5, 2018

Updated to reflect review comments.

@tgriesser tgriesser merged commit 66acbe7 into tgriesser:master Oct 6, 2018

1 of 2 checks passed

coverage/coveralls Coverage decreased (-0.8%) to 84.696%
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment