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

Close #2350 Fixes generated model relationship tests #2351

Merged
merged 1 commit into from
Oct 19, 2014

Conversation

jonathanKingston
Copy link
Member

This is one way of resolving this issue. The other would be to make the resolver allow for both cases in uses:[] which would probably be the better solution.

@@ -24,9 +24,9 @@ module.exports = {

if (/has-many/.test(dasherizedType)) {
var camelizedNamePlural = inflection.pluralize(camelizedName);
attrs.push(camelizedNamePlural + ': ' + dsAttr(camelizedName, dasherizedType));
attrs.push(camelizedNamePlural + ': ' + dsAttr(dasherizedName, dasherizedType));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@stefanpenner
Copy link
Contributor

This is one way of resolving this issue.

this is a a good solution

The other would be to make the resolver allow for both cases in uses:[] which would probably be the better solution.

My preference is only support 1 naming convention, dasherized. The motivation is to remove ambiguity and minimize confusion.

@@ -202,7 +202,11 @@ describe('Acceptance: ember generate', function() {
'bars:has-many',
'baz:belongs-to',
'echo:hasMany',
'bravo:belongs_to'
'bravo:belongs_to',
'barName:has-many',
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we also add foo-name:has-many as an example? (dasherized -> dasherized)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good call 👍 will add.

@jonathanKingston
Copy link
Member Author

@stefanpenner great thank you :D.

That AppVeyor issue appears to be the same that is on live.
I got all the tests working on ubuntu besides the other one I raised as an issue earlier which if no one else is seeing all should be fine.

@jonathanKingston
Copy link
Member Author

@stefanpenner all changes pushed, thanks again.

@stefanpenner
Copy link
Contributor

That AppVeyor issue appears to be the same that is on live.

Ya we haven't had time to get the windows build to be green yet for the acceptance tests. Hopefully soon.

stefanpenner added a commit that referenced this pull request Oct 19, 2014
Close #2350 Fixes generated model relationship tests
@stefanpenner stefanpenner merged commit 0fa4c1d into ember-cli:master Oct 19, 2014
@jonathanKingston jonathanKingston deleted the model-relationships branch October 19, 2014 22:20
@jonathanKingston
Copy link
Member Author

@stefanpenner 👍 thanks for merging.

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