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.

Sign up for GitHub

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

Jump to bottom

Allow passing of time via options #1592

Merged
merged 2 commits into from
Jul 18, 2017
Merged

Allow passing of time via options #1592

merged 2 commits into from
Jul 18, 2017

Conversation

mborst
Copy link
Contributor

@mborst mborst commented Jul 12, 2017

No description provided.

@mborst mborst mentioned this pull request Jul 12, 2017
Closed
@Playrom
Copy link
Contributor

Playrom commented Jul 12, 2017

Thanks for the PR

provide the tests and I'll merge to the codebase 👍

@Playrom
Copy link
Contributor

Playrom commented Jul 17, 2017

fixed with #1583 , released in version 0.10.4

@Playrom Playrom closed this Jul 17, 2017
@mborst
Copy link
Contributor Author

mborst commented Jul 17, 2017 •
edited
Loading

To be honest, I don't agree that the other PR fixes this issue. This PR is actually not even a fix, but rather a feature. I also think that while allowing to overwrite those fields explicitly as done in #1583 is fine, using this in place of the feature presented here is against the concept of the hasTimestamp feature, which handles the selection of the proper field to touch (createdAt vs updatedAt). Now, the client code always has to figure out if the operation will be an insert or an update, which is unnecessary in the solution presented here. WDYT?

@Playrom
Copy link
Contributor

Playrom commented Jul 17, 2017

sorry I was too hurry in closing this , but because of the absence of description I was mislead, really really sorry...

Can you articulate better the scope of this PR and what you mean for "passing of time"?

@Playrom Playrom reopened this Jul 17, 2017
@mborst
Copy link
Contributor Author

mborst commented Jul 18, 2017

The idea is that in some cases one wants to control what bookshelf thinks of as now, e.g. when you write state changes to two different places, like the db and a log. Now that #1583 is merged, you could manually do that, but that would not have the same convenience as using hasTimestamp, which handles updating the correct timestamp, i.e. updated_at on update, created_at on insert. IMHO, the feature proposed here is the sweet spot.

@Playrom
Copy link
Contributor

Playrom commented Jul 18, 2017

ok so with this change you could , while setting the options for the model, set that in reality now is yesterday

now I see the utility , sorry about that :)

@mborst
Copy link
Contributor Author

mborst commented Jul 18, 2017

I also noticed that the other PR was merged with failing tests. I fixed the indent there.

@Playrom
Copy link
Contributor

Playrom commented Jul 18, 2017

mmm nope the tests where ok in the other PR :\ strange that you saw failing ones

@mborst
Copy link
Contributor Author

mborst commented Jul 18, 2017

Ok, then linting is not part of the test suite. You could consider adding it.

@Playrom
Copy link
Contributor

Playrom commented Jul 18, 2017

you could open an issue and discuss about that :)

in the mean time I've merged it

@Playrom Playrom merged commit 36cf1b1 into bookshelf:master Jul 18, 2017
@mborst mborst deleted the allow-passing-of-time-via-options branch July 18, 2017 10:02
@mborst
Copy link
Contributor Author

mborst commented Jul 18, 2017

Thanks for taking the time!
I still can't quite figure out what happened with the other merge, because from a quick glance at the package.json it seems like the linting is run in the CI. But e367622 fixes wrong indentation that could be merged into master... will investigate when I find the time.

@Playrom
Copy link
Contributor

Playrom commented Jul 18, 2017

thank you very much :)

Playrom added a commit to Playrom/bookshelf that referenced this pull request Jul 18, 2017
@Playrom
Merge Remote
ea04a83 
commit c001e8a
Author: Giorgio Romano <Playrom@users.noreply.github.com>
Date:   Tue Jul 18 12:49:33 2017 +0200

    Link to the bookshelf#1600 discussion in README

    we added a temporary link in the README to direct all developers to the discussione about the future of bookshelf.js. It will be removed in a few weeks

commit 36cf1b1
Author: Michael Borst <mborst@users.noreply.github.com>
Date:   Tue Jul 18 12:01:47 2017 +0200

    Allow passing of time via options (bookshelf#1592)

    * Fix indent in timestamp method

    * Allow passing of time via options

commit cd9e9d2
Author: kirrg001 <katharina.irrgang@googlemail.com>
Date:   Mon Jul 17 17:01:05 2017 +0200

    Release 0.10.4
lyfeyaj added a commit to lyfeyaj/bookshelf that referenced this pull request Nov 6, 2017
@lyfeyaj
Merge https://github.com/bookshelf/bookshelf
ade939f 
* https://github.com/bookshelf/bookshelf: (43 commits)
  Fixed random test failure in test/integration/model.js
  Update .npmignore to exclude non-prod files
  Fix bookshelf#1662: move appropriate files into .github directory
  chore(package): update babel-eslint to version 8.0.1
  Add js formatting to "How do I debug?" at README
  Revert "Update dependencies to enable Greenkeeper 🌴 (bookshelf#1616)"
  Update dependencies to enable Greenkeeper 🌴 (bookshelf#1616)
  small registry plugin refactoring for easier read
  Build Project before testing in "npm test" script
  Update README.md (bookshelf#1537)
  New implementation for the setting of timestamps columns values
  Update CONTRIBUTING.md due to changes in MySQL 5.7 (bookshelf#1610)
  use OracleDB tests only when oracledb is installed (bookshelf#1609)
  added morphValues for morphTo relation (bookshelf#1326)
  Rename ISSUE-TEMPLATE.md to ISSUE_TEMPLATE.md
  Rename PR-TEMPLATE.md to PULL_REQUEST_TEMPLATE.md
  Revision to the contributing guidelines (bookshelf#1601)
  npm install knex@0.13 in the README (bookshelf#1604)
  Link to the bookshelf#1600 discussion in README
  Allow passing of time via options (bookshelf#1592)
  ...
@ricardograca ricardograca mentioned this pull request Mar 25, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
feature timestamps
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@mborst @Playrom @ricardograca