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

Adds custom automatic attribute names #23

Merged
merged 1 commit into from
Jul 14, 2015

Conversation

Esya
Copy link
Contributor

@Esya Esya commented Apr 11, 2015

Implements https://github.com/balderdashy/waterline/issues/754

This pull request adds a way for user to specify the attribute names they want for the automatic timestamp attributes, example :

collection = function() {};
collection.prototype = {
  tableName: 'Foo',
  autoCreatedAt: 'customCreatedAt',
  autoUpdatedAt: 'customUpdatedAt',
  attributes: {
    foo: 'string',
    bar: 'string'
  }
};

This way it will add the attributes customCreatedAt and customUpdatedAt with the right automatic timestamp behaviors.

@devinivy
Copy link

Haven't gotten a chance to try this out, but the PR looks good/complete to me!

@elad
Copy link

elad commented Apr 11, 2015

Thanks for taking the time to implement it!

Last time I looked at the relevant code I remember changes to multiple projects were necessary. I'm not sure if that's (still) the case, just putting it out here in case it helps anyone. :)

@devinivy
Copy link

@elad there is a matching PR in waterline: balderdashy/waterline#946
This reminds me: perhaps these options should be more explicitly normalized to either a string (for the property name) or false. I don't like that waterline and waterline-schema are both responsible for enforcing the default name for the auto-created properties. That should only occur in waterline-schema, and waterline should take for granted whatever waterline-schema spits-out. The defaults are enforced/decided only in this repo.

@Esya
Copy link
Contributor Author

Esya commented Apr 11, 2015

@devinivy I get your point, not quite sure how I should put that in practice though! I will give it a go later today or tomorrow.

Perhaps these options should be more explicitly normalized to either a string (for the property name) or false.

Well as autoCreatedAt and autoUpdatedAt are both set to true per default, I guess just mentioning this clearly on the documentation should be enough?

I don't like that waterline and waterline-schema are both responsible for enforcing the default name for the auto-created properties. That should only occur in waterline-schema, and waterline should take for granted whatever waterline-schema spits-out. The defaults are enforced/decided only in this repo.

I created createdAtProperty and updatedAtProperty flags (code below) but these are not exposed to waterline. Maybe I could just expose those to waterline so that it knows what name to pick, and all the logic regarding this would stay in waterline-schema ?

+  if(flags.autoCreatedAt) {
+    flags.createdAtProperty = (flags.autoCreatedAt === true) ? 'createdAt' : flags.autoCreatedAt;
+  }
+
+  if(flags.autoUpdatedAt) {
+    flags.updatedAtProperty = (flags.autoUpdatedAt === true) ? 'updatedAt' : flags.autoUpdatedAt;
+  }

@Esya
Copy link
Contributor Author

Esya commented Apr 13, 2015

@devinivy I've made the changes on both pullrequests so that now this is only enforced on waterline-schema.

Now waterline-schema will keep autoCreatedAt/autoUpdatedAt if they're false, if they're true it overrides with the default attribute names set in lib/attributes.js, and if they're a custom string it will keep that string as the attribute name.

@devinivy
Copy link

✔️ Awesome! Thanks a bunch @Esya

@dmarcelino
Copy link
Member

I like this ✅

I've pulled both PRs into my machine and all unit and integration tests pass.

particlebanana added a commit that referenced this pull request Jul 14, 2015
Adds custom automatic attribute names
@particlebanana particlebanana merged commit 682e50c into balderdashy:master Jul 14, 2015
@particlebanana
Copy link
Contributor

Looks good! Merged but we will need to publish 0.2.0 to npm for this. Let's do it all at once.

@Glavin001
Copy link

@particlebanana what would you say is a rough timeline for publishing 0.2.0 such that this PR is accessible in Waterline and Sails?

@tjpeden
Copy link

tjpeden commented Oct 27, 2015

+1 to the previous comment. What's the status of this?

@devinivy
Copy link

I defer to @particlebanana!

@golinmarq
Copy link

Can I use this already?

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