Skip to content

OptimisticLocking property: number --> number|string #86

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

Merged
merged 1 commit into from
Aug 7, 2017

Conversation

IzikLisbon
Copy link
Contributor

@IzikLisbon IzikLisbon commented Aug 6, 2017

http://docs.sequelizejs.com/manual/tutorial/models-definition.html

// Enable optimistic locking.  When enabled, sequelize will add a version count attribute
// to the model and throw an OptimisticLockingError error when stale instances are saved.
// Set to true or a string with the attribute name you want to use to enable.

IDefineOptions: as commented above -Set to true or a string.
Model.d.ts: if a user sets the version in IDefineOptions to a string (or not set it at all) it is probably to free the version field for something else and in that case it should not be forced to a number (This is the motivation for this pull request - I had a version field of type string which caused a build break).

http://docs.sequelizejs.com/manual/tutorial/models-definition.html

```
// Enable optimistic locking.  When enabled, sequelize will add a version count attribute
// to the model and throw an OptimisticLockingError error when stale instances are saved.
// Set to true or a string with the attribute name you want to use to enable.
```

IDefineOptions: as commented above -`Set to true or a string`.
Model.d.ts: if a user sets the version in IDefineOptions to a string it is probably to free the version field for something else, in that case it should not be forced to a number.
@codecov-io
Copy link

Codecov Report

Merging #86 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #86   +/-   ##
=======================================
  Coverage   95.65%   95.65%           
=======================================
  Files          66       66           
  Lines         759      759           
  Branches      106      106           
=======================================
  Hits          726      726           
  Misses         11       11           
  Partials       22       22

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 93e6281...c5b4d7e. Read the comment docs.

@RobinBuschmann
Copy link
Member

@IzikLisbon Hey, thanks for contributing. What does "something else" mean for Mode.d typings? What else could it be than a boolean, string or a number? Thank you

@IzikLisbon
Copy link
Contributor Author

IzikLisbon commented Aug 7, 2017

@RobinBuschmann It can be whatever the user decides it to be. Once the user is setting version in IDefineOption to a string, the version in Mode.d.ts can be of any type. Most likely that it will be a string or a number but there is no reason to force it.

@RobinBuschmann
Copy link
Member

Fair enough. Thanks again.

@RobinBuschmann RobinBuschmann merged commit 84a0538 into sequelize:master Aug 7, 2017
@Jendorski
Copy link

I cannot find IDefineOptions in sequelize-typescript ver ^2..1.3

import { IDefineOptions } from 'sequelize-typescript/lib/interfaces/IDefineOptions';

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.

4 participants