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

Use custom auto-attributes if specified in the model #946

Merged
merged 1 commit into from
Jul 15, 2015

Conversation

Esya
Copy link
Contributor

@Esya Esya commented Apr 11, 2015

This pullrequest goes along with the one I did for water-line schema here and implements #754

It allows users to specify custom names for the automatic timestamp attributes, they just need to pass a string instead of true or false.

Let me know if it is fine like this and I will update the documentation accordingly.

The Travis build will fail while the PR on waterline-schema is not merged, because my tests will not succeed, but all the older tests remain intact

Edit by @dmarcelino: Pending tasks:

@devinivy
Copy link
Contributor

Is there any reasonable way to refactor this so that the updatedAt and createdAt default values are enforced only by waterline-schema, and not by both waterline and waterline-schema? See my comment here: balderdashy/waterline-schema#23 (comment).

@Esya Esya force-pushed the custom-auto-attributes branch from 3fd671b to 70e1467 Compare April 13, 2015 12:04
@Esya
Copy link
Contributor Author

Esya commented Apr 13, 2015

@devinivy Done

Please be aware that now it needs the other PR on waterline-schema to be merged first.

@devinivy
Copy link
Contributor

✔️ Love it!

@dmarcelino
Copy link
Member

I'm good with this ✅

Just to be clear, in order to merge this PR we first need to:

Right?

@devinivy
Copy link
Contributor

And update docs?

@dmarcelino
Copy link
Member

Esya added a commit to Esya/waterline-docs that referenced this pull request Apr 17, 2015
This documentation goes with this pullrequest [waterline/#946](balderdashy/waterline#946) that allows users to specify custom attribute names for the automatic timestamps `createdAt` and `updatedAt`.
@Esya
Copy link
Contributor Author

Esya commented Apr 17, 2015

@dmarcelino that sounds about right! Just added a PR for the documentation, feel free to change the wording if needed.

@dmarcelino
Copy link
Member

It seems the linting changes created a merge conflict. We'll need to address those before merging this PR.

@particlebanana
Copy link
Contributor

Yeah looking at it now

@particlebanana
Copy link
Contributor

Rebased and submitted again as #1090

@particlebanana
Copy link
Contributor

@dmarcelino @devinivy should we also go ahead and look at adding the same logic for autoPK?

@devinivy
Copy link
Contributor

Seems like that would keep things nice and symmetrical!

@dmarcelino dmarcelino merged commit 70e1467 into balderdashy:master Jul 15, 2015
@dmarcelino
Copy link
Member

Yes, I think it makes sense for autoPK to behave in the same way.

Thanks @Esya! 🍻

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging this pull request may close these issues.

4 participants