change logic of timestamp "update_time" #1892
Conversation
new Post().fetch() will return a model, but here the code requires a collection.
the old logic seems a hot-fixed of a bug----by the way, the version on npm 0.13.3 was not up-to-date and will throw error when timestamps:['create_at',null]. so I have to use npm github ----I suggest to rewrite it this way. Additional discussion: 1) why check "this.hasChanged(createdAtKey)" when inserting? I don't quite understand. 2) if update-ts should be set to now when row is inserted? I think setting it to null on creation will provide more information----so people know this records has never been changed. But of course, this is more about convention.
try to update my fork
This reverts commit 7385667.
|
As you can see from the failing test case the reason for that code being that way is to enable user defined timestamps, so this PR is not going to be accepted unless you have something else to add. |
|
About your question number 2, that is how every other ORM I checked seems to work, so that's why it is that way. It seems to be an expectation at this point that both timestamp columns will be set when creating a new model. |
so the create and update are having the same logic of 3 conditions: a. key given; b. new model / value changed; c. ts not manually set;
I see what you meant. Never thought of that case. committed a new version. I think logically it's identical to the master branch, just a little more intuitive---hopefully.... |
|
by the way, do you know why npm is still using an old version? Or I should use some other tool to install bookshelf. |
in parse, when it doesn't response a tag column (e.g. only id is responsed), ||'[]' will add will set tags to empty, which in my opinion is not true. Similarly, in format, if tags is not provides, it should not be updated.
|
@eerenyuan What do you mean by an old version? The most recent version on npm is 0.13.3. A new version is almost ready and will be released soon. |
the current logic seems a hot-fixed of a old bug----by the way, the version on npm 0.13.3 was not up-to-date and will throw error when timestamps:['create_at',null].
although it does solve the problem when updateAtKey is null by adding the marked code below. The current code seems unnecessarily complex.
const setUpdatedAt = updatedAtKey && this.hasChanged(updatedAtKey)
if ( ******updatedAtKey && *******(isNewModel && !setUpdatedAt || this.hasChanged() && !setUpdatedAt)) {
attributes[updatedAtKey] = now;
}
I suggest to rewrite it this new way.
Additional discussion: