Fix AMS::Model accessor vs. attributes mutation

[bf4]

So far when I fix it, other tests break, but I think that's because the subclass 'Model' has that crazy method_missing

[mention-bot]

@bf4, thanks for your PR! By analyzing the annotation information on this pull request, we identified @dubadub and @yevhene to be potential reviewers

[richmolj]

This comment notes how to fix. Though I prefer we get rid of this thing

[NullVoxPopuli]

👍 for fixing by just renaming AMS::Model to AMS::Serializable

[richmolj]

I am not sure how that would fix it 😉

[NullVoxPopuli]

@bf4, @richmolj I fixed the tests.

Had to add send if respond_to?

and removed the attributes comparison, because it doesn't make sense when using attr_accessor (which is what the send covers)

[NullVoxPopuli]

hopefully the tests pass and it'll be ready to merge

[NullVoxPopuli]

darn testing gems not working on jruby

[richmolj]

@NullVoxPopuli I tried to give this an honest review, though this class still seems like worthless cruft to me

[richmolj]

It looks like this test is about a change via accessor instead of constructor. Maybe the test name could be a little more descriptive in that regard?

[richmolj]

Why is this needed?

[richmolj]

Hm, I'm not totally sure this would work. You'd get into this scenario:

class Post < ActiveModelSerializers::Model
  attr_accessor :title
end

post = Post.new('title' => 'my title')
post.read_attribute_for_serialization(:title) # => nil

[NullVoxPopuli]

that's true. I think that's just another point for the current AMS::Model being broken. :-(

Do you have a PR for your module idea?

[richmolj]

Nope, I was waiting to get approval of the RFC. Because what would that module be? Would it contain the constructor logic (in which case we are in the business of overriding the superclass's constructor which is gross)? And if not, what would it include?

As far as I can tell it would just be alias read_attribute_for_serialization send, and in that case I'm not sure what the point of it is.

So, TBH AMS::Model is nonsensical to me, which means I don't think I can touch the code or enhance the documentation in a meaningful way. I created the RFC to try and improve that situation, but doesn't look like it is getting any traction.

[richmolj]

FWIW you can fix the current class with something like this and this. That got the tests passing, including the scenario above, though it's just POC code.

[bf4]

consider:

    class_attribute :attribute_names
    self.attribute_names = []

    def self.attributes(*names)
      attr_accessor(*names)
      self.attribute_names += names
    end

    def attributes
      attribute_names.each_with_object({}) {|field, result|
        result[field] = public_send(field)
      }
    end

[bf4]

Closed by #1984