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

change logic of timestamp "update_time" #1892

Merged
merged 9 commits into from Dec 9, 2018
Merged

Conversation

@eerenyuan
Copy link
Contributor

@eerenyuan eerenyuan commented Sep 15, 2018

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:

  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.
eerenyuan added 5 commits Sep 13, 2018
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.
@ricardograca
Copy link
Member

@ricardograca ricardograca commented Sep 15, 2018

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.

@ricardograca
Copy link
Member

@ricardograca ricardograca commented Sep 15, 2018

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;
@eerenyuan
Copy link
Contributor Author

@eerenyuan eerenyuan commented Sep 15, 2018

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.

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....

@eerenyuan
Copy link
Contributor Author

@eerenyuan eerenyuan commented Sep 15, 2018

by the way, do you know why npm is still using an old version? Or I should use some other tool to install bookshelf.

eerenyuan added 2 commits Sep 19, 2018
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.
@ricardograca
Copy link
Member

@ricardograca ricardograca commented Nov 22, 2018

@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.

Copy link
Member

@ricardograca ricardograca left a comment

These changes seem fine to me, and the condition is refactored in order to make it clearer, so I'm merging this.

@ricardograca ricardograca added this to To do in Version 1.0.0 via automation Dec 9, 2018
@ricardograca ricardograca moved this from To do to In progress in Version 1.0.0 Dec 9, 2018
@ricardograca ricardograca merged commit ec58b8d into bookshelf:master Dec 9, 2018
1 check passed
Version 1.0.0 automation moved this from In progress to Done Dec 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Version 1.0.0
  
Done
Linked issues

Successfully merging this pull request may close these issues.

None yet

2 participants