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

fix model relationship name generation #3885

Merged
merged 1 commit into from
Nov 3, 2015

Conversation

gabrielgrant
Copy link
Contributor

It is not currently possible to generate a model with a relationship named differently from the model's name (despite the help indicating it should be)

This patch brings the code's behavior in line with the docs.

I've also filed ember-cli/ember-cli#4986 as a hold-over until ember-cli/ember-cli#4909 lands

(not sure how closely you monitor ED PRs, so cc: @stefanpenner )

@igorT
Copy link
Member

igorT commented Oct 26, 2015

Not sure what you mean, can you please explain and/or write a test?

@gabrielgrant
Copy link
Contributor Author

To clarify, this is a bug in the new ember-cli blueprints that @stefanpenner is bringing into ED from the ember-cli repo (hence CCing him and opening the PR against this branch)

The docs for the model blueprint state that ember generate model taco filling:belongs-to:protein toppings:has-many:toppings name:string price:number misc should result in the following model:

import DS from 'ember-data';
export default DS.Model.extend({
  filling: DS.belongsTo('protein'),
  toppings: DS.hasMany('topping'),
  name: DS.attr('string'),
  price: DS.attr('number'),
  misc: DS.attr()
});

But instead, this is what that command actually generates:

import DS from 'ember-data';

export default DS.Model.extend({
  filling: DS.belongsTo('filling'),  // <------ should be 'protein'
  toppings: DS.hasMany('topping'),
  name: DS.attr('string'),
  price: DS.attr('number'),
  misc: DS.attr()
});

That is, the name of the model gets ignored, and instead just pulled from the name of the field. This patch fixes the issue in the blueprint, but is dependent on ember-cli/ember-cli#4986 to fix ember-cli's argument parsing code because it currently just throws out everything after the second : (this PR shouldn't break anything without that one, but it won't fix the issue until that lands)

Sorry for lack of tests; I don't believe ember-cli has any testing infrastructure for it's blueprint generation code, and putting that in place should probably be a separate PR.

@stefanpenner
Copy link
Member

(not sure how closely you monitor ED PRs, so cc: @stefanpenner )

thanks for the CC, reviewing now.

@stefanpenner
Copy link
Member

looks good, we should add tests on the ember-cli side: ember-cli/ember-cli#4986 (comment)

We are still working to extract common tests for this PR (@trabus has been doing some good job here, maybe he has some insight)

homu added a commit to ember-cli/ember-cli that referenced this pull request Oct 26, 2015
fix model relationship name generation

It is not currently possible to generate a model with a relationship named differently from the model's name ([despite the help indicating it should be](https://github.com/ember-cli/ember-cli/blob/master/blueprints/model/HELP.md))

This patch brings the code's behavior in line with the docs.

The blueprint part of this patch will be obsoleted if #4909 lands, so I've also PRed the changes to the blueprint in ED: emberjs/data#3885 

(the arg-parsing-fix part of this patch is still needed to pass a distinct name, though)
stefanpenner added a commit that referenced this pull request Nov 3, 2015
fix model relationship name generation
@stefanpenner stefanpenner merged commit f5e745d into emberjs:addonified Nov 3, 2015
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