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

Setting a key using send should return the new value #557

Closed
wants to merge 1 commit into from

Conversation

tjwp
Copy link
Contributor

@tjwp tjwp commented Dec 12, 2013

There seems to be an unnecessary line at the end of internal_write_key
that results in send("#{key_name}=", value) returning nil. This breaks
compatibility with 0.12.

The @attributes variable that is set to nil in the last line of
internal_write_key does not appear to be
referenced elsewhere.

There seems to be an unnecessary line at the end of `internal_write_key`
that results in `send("#{key_name}=", value)` returning nil. This breaks
compatibility with 0.12.

The `@attributes` variable that is set to nil in the last line of
`internal_write_key` does not appear to be
referenced elsewhere.
@tjwp
Copy link
Contributor Author

tjwp commented Dec 12, 2013

The Travis failure for JRuby looks unrelated to this change.

@cheald
Copy link
Member

cheald commented Dec 12, 2013

IIRC, @attributes is relevant to ActiveModel - I'll see what I can figure out, but I don't think it's valid to just remove it.

@cheald cheald closed this in 04d7a1c Dec 12, 2013
@mtsmfm
Copy link
Contributor

mtsmfm commented Jan 2, 2019

@cheald Sorry, could you tell me why is this @attributes needed?
I'm trying to change MM to support newer version of ActiveModel and I'm wondering if we can remove that because in Rails 5.2 it breaks ActiveModel attribute behavior.

@cheald
Copy link
Member

cheald commented Jan 2, 2019

I honestly don't recall; if you can get it passing spec suites without it, then go nuts! I seem to think it was something that fulfilled an ActiveModel requirement, though I couldn't tell you why.

mtsmfm added a commit to mtsmfm/mongomapper that referenced this pull request Jan 2, 2019
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.

None yet

3 participants