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

Merge attrs from superclasses into their subclasses. #3055

Merged
merged 1 commit into from
May 8, 2015

Conversation

omghax
Copy link
Contributor

@omghax omghax commented May 8, 2015

This PR defines attrs as a mergedProperty, so it can be inherited from superclasses. This is useful if you have a polymorphic type and want to define a base serializer for common attributes.

igorT added a commit that referenced this pull request May 8, 2015
Merge `attrs` from superclasses into their subclasses.
@igorT igorT merged commit 7d7c55d into emberjs:master May 8, 2015
@igorT
Copy link
Member

igorT commented May 8, 2015

Thanks

@pangratz
Copy link
Member

pangratz commented May 8, 2015

Nice

@igorT
Copy link
Member

igorT commented May 8, 2015

Would be nice to point out somewhere in the docs

On Fri, May 8, 2015 at 11:13 PM, Clemens Müller [email protected]
wrote:

Nice


Reply to this email directly or view it on GitHub
#3055 (comment).

@pangratz
Copy link
Member

pangratz commented May 8, 2015

What about merging hashes?

var BaseSerializer = DS.JSONSerializer.extend({
  attrs: {
    comments: {
      key: "le_comments"
    }
  }
});

var PostSerializer = BaseSerializer.extend({
  attrs: {
    comments: {
      anotherOptionForCustomSerializer: true
    }
  }
});

results currently in the following attrs hash:

{
  comments: {
    anotherOptionForCustomSerializer: true
  }
}

Do we want the result to be:

{
  comments: {
    key: "le_comments",
    anotherOptionForCustomSerializer: true
  }
}

??

@igorT
Copy link
Member

igorT commented May 8, 2015

I feel like having to look at multiple classes to be sure of what the hash
is pretty error prone. Not sure this is a really common use case and would
be confusing. Open to being persuaded otherwise

On Fri, May 8, 2015 at 11:22 PM, Clemens Müller [email protected]
wrote:

What about merging hashes?

var BaseSerializer = DS.JSONSerializer.extend({
attrs: {
comments: {
key: "le_comments"
}
}
});
var PostSerializer = BaseSerializer.extend({
attrs: {
comments: {
anotherOptionForCustomSerializer: true
}
}
});

results currently in the following attrs hash:

{
comments: {
anotherOptionForCustomSerializer: true
}
}

Do we want the result to be:

{
comments: {
key: "le_comments",
anotherOptionForCustomSerializer: true
}
}

??


Reply to this email directly or view it on GitHub
#3055 (comment).

@pangratz
Copy link
Member

pangratz commented May 8, 2015

Yeah, you might be right that this is not a common use case ...

@omghax
Copy link
Contributor Author

omghax commented May 8, 2015

My mental model of attrs was that it behaved like actions in Ember, so I was a little surprised when that wasn't the case. Granted actions is flat, unlike attrs. I'm not in favor of recursively merging though, it seems like that would add more surprises.

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