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

Naming cleanup #4601

Merged
merged 1 commit into from
Oct 28, 2016
Merged

Naming cleanup #4601

merged 1 commit into from
Oct 28, 2016

Conversation

runspired
Copy link
Contributor

Attempting to enforce consistent use of

  • internalModel: for InternalModel instances
  • record: for DS.Model instances
  • modelClass: for extended DS.Model
  • kind: for whether something is a belongsTo/hasMany
  • modelName: for the name of a modelClass

@runspired runspired force-pushed the feat/internal-model-round2 branch 2 times, most recently from 2135379 to 6106341 Compare October 22, 2016 13:41
@runspired runspired force-pushed the feat/internal-model-round2 branch from 6106341 to 196ca7a Compare October 22, 2016 14:29
@runspired runspired changed the title [WIP] Naming cleanup Naming cleanup Oct 22, 2016
@runspired runspired force-pushed the feat/internal-model-round2 branch from 4620406 to 73a3185 Compare October 22, 2016 16:36
@bmac
Copy link
Member

bmac commented Oct 24, 2016

I'm excited for this change. 🎉

assert.expectAssertion(function() {
internalModel.materializeRecord();
}, /more than once/);
});
Copy link
Member

Choose a reason for hiding this comment

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

What's the reasoning behind removing this test?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it's now impossible to trigger this assertion

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@pangratz I think internal model needs tests, but that's a separate PR, I removed this test because it's now not possible to materialize a record more than once.

@bmac bmac merged commit 2ac536d into emberjs:master Oct 28, 2016
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