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

Fix consistency issue between ActiveModel::Conversion and ActiveModel::Lint #45338

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

santib
Copy link
Contributor

@santib santib commented Jun 12, 2022

Summary

ActiveModel::Lint says #to_key should return nil if #persisted? is false, but ActiveModel::Conversion that implements #to_key doesn't care about #persisted? but it actually cares about #id.

The primary change in the PR is on ActiveModel::Lint#test_to_key to set the #id method on the model instead of the #persisted? method, so it always passes when ActiveModel::Conversion is included in a class.

@matthewd
Copy link
Member

Relevant context for why it used to work that way is here:

# Note the default implementation uses persisted? just because all objects
# in Ruby 1.8.x responds to <tt>:id</tt>.

This seems like an improvement, though I am conscious of the fact that this could result in a model that previously passed the lint now failing it, because we're changing the implemented expectation to better match what the documentation claimed (and, almost incidentally, what our default implementation does). Hopefully that's okay in practice? 🤷🏻‍♂️

@santib
Copy link
Contributor Author

santib commented Jun 18, 2022

Relevant context for why it used to work that way is here:

# Note the default implementation uses persisted? just because all objects
# in Ruby 1.8.x responds to <tt>:id</tt>.

Wow, very interesting, I wasn't aware of that.

This seems like an improvement, though I am conscious of the fact that this could result in a model that previously passed the lint now failing it, because we're changing the implemented expectation to better match what the documentation claimed (and, almost incidentally, what our default implementation does). Hopefully that's okay in practice? 🤷🏻‍♂️

hmm yeah, that's a valid concern. Other options could be:

  1. Making ActiveModel::Conversion use persisted? in the to_key method
  2. Changing the ActiveModel::Lint test to mock both persisted? and id to return false and nil respectively

I guess the safest one is 2. ?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants