add class method "increment" #7394
Conversation
|
the test fails in a strange line of code. |
lib/query-interface.js
Outdated
|
|
||
| options.type = QueryTypes.UPDATE; | ||
| options.instance = instance; |
There was a problem hiding this comment.
This line is what makes returning: true work - It shouldn't be removed :)
test/unit/increment.test.js
Outdated
| stub.restore(); | ||
| }); | ||
|
|
||
| it('should allow increments even if options are not given', function () { |
There was a problem hiding this comment.
We should require where to be defined, like we do for update, so users don't accidentally update all rows in the database
lib/model.js
Outdated
| } | ||
|
|
||
| if (!options.increment) { | ||
| return this.sequelize.getQueryInterface().decrement(this, this.getTableName(options), values, where, options).then(affectedRows => { |
There was a problem hiding this comment.
The then handlers are duplicated here
|
Thanks for the quick help and the test finally passes! I'd like to receive more suggestions for improving this PR since this is the very first contribution I make for open source. :-) |
|
@hieven I don't have any other comments I think, code style looks good, and I like your thoroughness in testing :) |
Basically moving code from instance method `increment` to class method `increment`. close #6359
|
@janmeier |
Pull Request check-list
Please make sure to review and check all of these items:
npm run testornpm run test-DIALECTpass with this change (including linting)?Futurein the changelog?NOTE: these things are not required to open a PR and can be done afterwards / while the PR is open.
Description of change
I move the instance method
incrementcode to be class methodincrement.The original instance method
incrementwill simply addthis.where()tooptions.whereand then callthis.constructor.increment().This is the very first open source contribution I do.
Please give me any advices to improve this PR :-)
I will write document and changelog if this PR is acceptable
close #6359