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

define methods for properties and associations getter/setter #300

Merged

Conversation

senid231
Copy link
Member

Description

It's a bad practice to dynamically create methods when accessing method missing, so I changed a behavior of of static define attributes.

Generally I add accessors for each attribute that defined with property class method and relactionships that defined with has_one, has_many, and belongs_to class methods

performance may be decreased comparing to previous version unless you define properties for all accessed attributes

@jsmestad
Copy link
Collaborator

I like the change. There does seem to be quite a bit of style change embedded in this PR though. I'd suggest splitting out the feature change from the code style changes.

To explain that a little more:

Given the lack of documentation on this project, that means folks often rely on reading the source code to find their answer. As a result the code-style being uniform has a greater impact on library comprehension. - If you want to suggest the style changes, we probably would want to do it across the library.

@gaorlov
Copy link
Collaborator

gaorlov commented Sep 24, 2018

Thanks for your work! These are solid changes.
My comments are similar to @jsmestad:
Can we move the association logic out of Resource into a concern-like module? Associations for example?

It may be easier to read & test if that code is contained in a single module.

The way i've done it before it to have an Associable concern that defines has_one, etc functions and handles all that business. Something like

module JsonApiClient
  module Associatable
    extend ActiveSupport::Concern

    included do

      class_attribute :__associations
      self.__associations = {}

      attr_accessor :__cached_associations

      class << self

        def belongs_to( name, opts = {} )
          process Associations::BelongsTo.new( self, name, opts )
        end

        def has_one( name, opts = {} )
          process Associations::HasOne.new( self, name, opts )
        end

        def has_many( name, opts = {} )
          process Associations::HasMany.new( self, name, opts )
        end
        
        protected

        def process( association )
          associate association
          methodize association
        end

        def associate( association )
          self.__associations = __associations.merge association.name => association
        end

        def methodize( association )
          define_method association.name do
            self.__cached_associations ||= {}

            unless self.__cached_associations.has_key? association.name
              self.send( "#{association.name}=", association.fetch( self ) )
            end
            self.__cached_associations[association.name]
          end

          define_method "#{association.name}=" do | value |
            self.__cached_associations ||= {}
            self.__cached_associations[association.name] = value
          end
        end
      end
    end
  end
end

Does that make sense?

Functionally, again, this is solid.

@senid231
Copy link
Member Author

senid231 commented Sep 25, 2018

@jsmestad thanks for your review. I agree with you. I'll remove style changes and introduce styling refactor MR later.
I want to use rubocop here cause I've found it very useful in my other projects.

@senid231
Copy link
Member Author

@gaorlov thanks for your review. I agree that code will be more readable if we move associations logic into separate concern.

@gaorlov
Copy link
Collaborator

gaorlov commented Sep 25, 2018

How can I help?

@senid231 senid231 force-pushed the smart-performance-optimization branch from f5941c5 to 85ee5f9 Compare October 3, 2018 09:21
@senid231
Copy link
Member Author

senid231 commented Oct 3, 2018

@jsmestad @gaorlov I've updated the PR
please take a look

Copy link
Collaborator

@gaorlov gaorlov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks really solid. My only comment is whether we need some code in method_missing. Nicely done.

nil
end

def method_missing(method, *args)
association = association_for(method)
relationship_definition = relationship_definition_for(method)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since creating an association generates the accessor methods, will a relationship ever get into method_missing?
If not, do we need this code?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good catch)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@gaorlov, I've changed it. Take a look

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Nicely done.


end
end
end
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, man.
That's so much nicer to read! Strong work!

@senid231 senid231 force-pushed the smart-performance-optimization branch from 85ee5f9 to 8d3c942 Compare October 5, 2018 07:32
@senid231 senid231 force-pushed the smart-performance-optimization branch from 8d3c942 to 3048ea3 Compare October 5, 2018 07:51
@senid231 senid231 mentioned this pull request Oct 5, 2018
Copy link
Collaborator

@gaorlov gaorlov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks solid

@senid231 senid231 merged commit dda66ea into JsonApiClient:master Oct 8, 2018
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