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

Supply Date.now() #1551

Closed
mborst opened this issue Apr 12, 2017 · 9 comments
Closed

Supply Date.now() #1551

mborst opened this issue Apr 12, 2017 · 9 comments
Labels

Comments

@mborst
Copy link
Contributor

mborst commented Apr 12, 2017

I maintain an app that uses a software supplied timestamp for all timestamp fields in the database, among them the create and update timestamps.

Doing this while still using the hasTimestamp magic requires patching this line to use options[...] instead of new Date().

I could provide a PR if that seems like a useful feature to have in bookshelf.

@kirrg001
Copy link
Contributor

@mborst You can use the format fn to modify the dates before they are written into the database.

@mborst
Copy link
Contributor Author

mborst commented Apr 20, 2017

Thanks! But I think using format here is kind of sub-optimal. Seems very unintuitive to really modify the data there. Plus how would I pass in the date into format?
For now I have written a plugin that patches hasTimestamp in the suggested way, which makes the most sense imho. Or do you disagree with the general idea of this suggestion?

@kirrg001
Copy link
Contributor

The options object is usually used for very general options e.g. transaction, method.
That's why i am not sure your suggestion fits good. The plugin you have written is also a good choice 👍 Otherwise the format function can be used to transform the created_at/updated_at into the format you need.

@mborst
Copy link
Contributor Author

mborst commented Apr 20, 2017

It's not about formatting it, I would really change it! Plus, I would also need to pass the timestamp I want to use into format, which would be a far bigger change.
To maybe make what I want to achieve more clear:
bsiddiqui/bookshelf-paranoia#21

@Playrom
Copy link
Contributor

Playrom commented Jun 29, 2017

maybe this has been fixed with #1583 , is it the same problems, am I correct?

@Playrom Playrom added the bug label Jun 29, 2017
@mborst
Copy link
Contributor Author

mborst commented Jun 29, 2017

Halfway. I also want to use it for the updated_at timestamp. That's why I wrote a plugin that applies the patch mentioned above (picking the user supplied date from options). That way, I can use the same interface as the paranoia plugin allows (see here)

@Playrom
Copy link
Contributor

Playrom commented Jun 29, 2017

ok!

can you provide a PR in the same style as #1583 but for the update_at timestamp?

I will merge it to the master :)

@mborst
Copy link
Contributor Author

mborst commented Jul 12, 2017

Ok, so this would be the PR: #1592. If you agree with it in principle, I could provide tests and documentation.

@Playrom
Copy link
Contributor

Playrom commented Jul 12, 2017

closing here, discuss in #1592

@Playrom Playrom closed this as completed Jul 12, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants