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

[CLEANUP Beta] Cleanup Ember.get #11746

Merged
merged 2 commits into from Jul 15, 2015
Merged

[CLEANUP Beta] Cleanup Ember.get #11746

merged 2 commits into from Jul 15, 2015

Conversation

ghost
Copy link

@ghost ghost commented Jul 14, 2015

Resolves #11733

@@ -51,6 +51,8 @@ export let UNHANDLED_GET = symbol('UNHANDLED_GET');
@public
*/
export function get(obj, keyName) {
Ember.assert(`Get must be called with two arguments; an object and a property key`, arguments.length >= 2);

Copy link
Author

Choose a reason for hiding this comment

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

It seems that _getPath (here), calls get with three arguments, can anyone explain me why?

Copy link
Member

Choose a reason for hiding this comment

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

Looks like it should not.

@ghost
Copy link
Author

ghost commented Jul 15, 2015

There is an error in the test suite

@@ -51,24 +50,17 @@ export let UNHANDLED_GET = symbol('UNHANDLED_GET');
@public
*/
export function get(obj, keyName) {
Ember.assert(`Get must be called with two arguments; an object and a property key`, arguments.length === 2);
Ember.assert(`Cannot call get with '${keyName}' on an undefined object.`, obj !== undefined && obj !== null);
Ember.assert(`Cannot call get with ${keyName} key.`, typeof keyName === 'string');
Copy link
Contributor

Choose a reason for hiding this comment

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

This assertion could be clearer with something like The key provided to get must be a string, you passed ${keyName}

Copy link
Author

Choose a reason for hiding this comment

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

I just took the existing message. But if that's better I could change it.

Copy link
Sponsor Member

Choose a reason for hiding this comment

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

Seems like a good improvement

@mixonic
Copy link
Sponsor Member

mixonic commented Jul 15, 2015

This is sweet 👏

@ghost
Copy link
Author

ghost commented Jul 15, 2015

Test suite fails on Ember.view using global lookup for the emptyView property.
That feature has been deprecated, I have created PR #11754, which removes the feature.

@ghost ghost changed the title [CLEANUP Beta][WIP] Cleanup Ember.get [CLEANUP Beta] Cleanup Ember.get Jul 15, 2015
@stefanpenner
Copy link
Member

Lgtm

@mixonic
Copy link
Sponsor Member

mixonic commented Jul 15, 2015

@rwjblue please suggest squash or merge if you are happy as is.

- Removes support for global lookup with get.
- Removes support for get with this in paths.
- Removes support for get with null as first parameter.
- Adds an assertion that get always has to be called with two
  parameters.
@ghost
Copy link
Author

ghost commented Jul 15, 2015

I squashed all but one commit.
That commit might belong in another PR as it's more of an bug fix I think.

stefanpenner added a commit that referenced this pull request Jul 15, 2015
@stefanpenner stefanpenner merged commit 6413ad8 into emberjs:master Jul 15, 2015
@ghost ghost mentioned this pull request Jul 15, 2015
@ghost ghost deleted the cleanup-ember-get branch July 17, 2015 16:32
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.

5 participants