fog / fog-core

fog's core, shared behaviors without API and provider specifics
MIT License
45 stars 95 forks source link

Model methods #281

Closed jsirex closed 1 year ago

jsirex commented 1 year ago

Please, do not squash commits when (if) merging.

Add methods #create, #update, #save, #destroy explicitly. Be more consistent with return values. Add filter_attributes helper method.

Commit messages below:


Model methods like #save or #destroy are implicit: they only mentioned in README but explicitly used by framework. Let define them also explicitly so developer knows exactly what to implement.

This change is not breaking change, because either method is already defined in child class or is not used at all.

Another goal is to make return values consistent.


It is just handy method to easily get a sub-set of attributes. For example, my model has 10+ attributes, but when creating/updating model I need only a few of them:

Code: ... attribute :name attribute :value attribute :comment attribute :type ... service.create_entity(filter_attributes(:name, :value, :type))

jsirex commented 1 year ago

Also I have some questions:

  1. Should we drop any code related to ruby 1.x? According to README 2.x now required
  2. Should I update rubocop configuration to use 2.x syntax?
geemus commented 1 year ago

Thanks for the updates. With filter_attributes it's a method that you would call on a instance, right? So it might look something like this:

existing_object = service.collection.all.first
copycat = service.collection.create(existing_object.filter_attributes(:name, :value))

I think that makes sense to me, I wasn't exactly following the initial example above, so I wanted to check in. Is something like what I wrote what you had in mind, or am I still missing something?

I think it would be fine to drop 1.x code and update rubocop, but it may make sense to do that in distinct PRs.

jsirex commented 1 year ago

distinct PRs - ok, got it. I'll split stuff and "spam" you with PRs :)

regarding filter_attributes - yes, it is in Fog::Attributes::InstanceMethods module.

Use case: handy method to quickly get a sub-hash of Model's attributes. Actually my real cases:

# Quickly build { id: model.identity }
class AModel
....
  def create
        requires :account_id, :list_id, :identity
        items = [filter_attributes(:id)]
        service.delete_list_items(account_id, list_id, items)
  end

# Select many attributes
class DNSRecord
 attributes :many
 attributes :many_of
 attributes :many_of_them
 attributes :many_of_them_really
...
      def create
        requires :zone_id, :name, :content, :type
        # build a Hash in one shot. I can't use self.attributes because some of them won't be recognized by create_dns_record
        attrs = filter_attributes(:name, :content, :type, :ttl, :proxied, :comment, :tags)
        data = service.create_dns_record(zone_id, attrs)
         merge_attributes(data)
      end

Your example also makes sense to me.

In a collection I have the following real case:

class AModel
...
  def pure_attributes
    filter_attributes(:id, :value)
  end
....
end

# in collection
class ACollection
...
        def update!
          requires :account_id, :list_id

          # just pleasure to read this: build array of Hash of subset of attributes.
          items = map(&:pure_attributes)
          service.update_all_list_items(account_id, list_id, items)

          all
        end
geemus commented 1 year ago

Thanks for the extra examples, just wanted to be sure I understood the intention.

Yeah, with breaking things up it's always a tradeoff, but I usually feel like it's a bit easier to discuss and manage the more spammy way. Thanks again!

Do you feel like this PR is ready now then, or is it still WIP?

jsirex commented 1 year ago

Let it be wip. I'll split out logic in other PRs. Just busy a bit last couple of days.

geemus commented 1 year ago

Yeah, no worries and no hurry, just wanted to see what you had in mind.

Just let me know when you get a chance and I'll review.

jsirex commented 1 year ago

I think this PR is ready for review/merge