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

Issue 85 create required attributes on update #96

Conversation

aloof-ruf
Copy link
Contributor

This pull request is meant to address Issue #85. There is a lot going on here...

Model.update now has a new touch. If you pass a null or undefined key, then there is a check for key defaults. If the key attributes have defaults, those will be used. If they don't, then an error is thrown. This check is currently not in place.

timestamps (as defined in the schema) are now updated by default. You can turn this behaviour off by:

  1. Not having timestamps
  2. setting the updateTimestamps option to false

But, most importantly, this PR now has the update also conditionally update required attributes. There is one catch though: required attributes that do not have a default value will throw an error on update if no value is provided for them.

I also added some tests for Module.update

- user can now choose whether or not to update timestamps specified with
the 'timestamps' option (defined when making schema) on update.
Passing null or undefined for the key parameter in Model.update will now
have it check default values for those keys before erroring out.
@brandongoode
Copy link
Contributor

First -- Sorry for the delayed response.

It looks great.

The one thing I'm unsure about is createRequired defaulting to true, because it changes the behavior of existing code. I don't want to change to break a bunch of peoples existing code or worse overwrite their data.

What are your thoughts on changing the createRequired default to false? What about creating global option to set the default behavior?

@aloof-ruf
Copy link
Contributor Author

Setting it to false is fine with me. I agree with you, these changes should allow for the feature instead of imposing it on existing code. Looks like I have merge conflicts to resolve anyway

- Table.js had a deferred rejection with just a string (changed to an
error object)
- updated model tests to reflect createRequired's default of false
@brandongoode brandongoode added this to the v0.8.0 milestone Jan 17, 2017
@brandongoode
Copy link
Contributor

Thanks a bunch for this PR.

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

Successfully merging this pull request may close these issues.

2 participants