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

added Model.primary(name) to set the key #50

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ianstormtaylor
Copy link
Contributor

I've been wanting to have a nicer way to set the primary key for a model. Instead of having to break the nice chainable API to do:

var Model = model('post')
  .attr('id')
  .attr('slug')
  .attr('title')
  .attr('body');

Model.primaryKey = 'slug';

Instead, we can add a chainable method to set the primaryKey. Note that I removed the model[attr].primaryKey = true piece of code because it seemed unnecessary and wasn't being used anywhere, so I figured it was a bit of feature creep for now.

Now setting a primaryKey would look like:

var Model = model('post')
  .attr('id')
  .attr('slug')
  .attr('title')
  .attr('body')
  .primary('slug');

But then I realized that there might even be a better way using the extra options we can tie to a specific property, which would look like this:

var Model = model('post')
  .attr('id')
  .attr('slug', { primary: true })
  .attr('title')
  .attr('body');

That last option seems the cleanest to me, and it builds on the existing API. (Note: that's not what's in the PR, so let me know if you like that one better and I'll submit another with it.)

@tj
Copy link
Member

tj commented Aug 26, 2013

hmmm yeah I like the last one too

@ianstormtaylor
Copy link
Contributor Author

how do you feel about changing primaryKey to primary? just worried about having both in there since the method is primary() and the current prop is primaryKey.

@tj
Copy link
Member

tj commented Aug 26, 2013

SGTM

@yocontra
Copy link

yocontra commented Feb 6, 2014

Any updates on this?

@ianstormtaylor
Copy link
Contributor Author

when #58 gets merged (sorry getting distracted) this will be fixed

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.

3 participants