Skip to content

Instantly share code, notes, and snippets.

@joseph-ravenwolfe
Forked from nilbus/request.md
Created November 11, 2015 22:40
Show Gist options
  • Save joseph-ravenwolfe/06514a8d54c5071de939 to your computer and use it in GitHub Desktop.
Save joseph-ravenwolfe/06514a8d54c5071de939 to your computer and use it in GitHub Desktop.
Proposed changes to tcrayford/Values

This gem is great. Thanks for releasing it!

We're running into 2 issues using Values, however.

  1. Inheritance from other classes is impossible
  2. No support for optional arguments to ValueClass.with

What I'm suggesting is a significant deviation from current usage and would require a major version release. That said, I think the benefits are worth it. Please review these ideas, and let me know if you would consider accepting future Pull Requests for these changes into the project, providing that the changes are simple and elegant enough to stay in line with the project's goal for simplicity. If not, we'll fork and release under a different name.

Inheritance

Rather than inheriting from Value.new, we think it would be more appropriate to follow the pattern I see in the Equalizer gem:

class GeoLocation
  include Value.new(:latitude, :longitude)
end

instead of

class GeoLocation < Value.new(:latitude, :longitude)
  # Cannot use Values and inherit from a different superclass
end

Optional Attributes

There are many valid use cases where a value object's attributes are optional. Being required to specify nil for these is frustrating and not elegant.

Given the use case below, consider the 4 following approaches to allow this:

Filter.with(matcher: /available/, limit: 100)
Filter.with(matcher: /available/)
  1. Consider all attributes optional, with no defaults:

    class Filter
      include Value.new(:matcher, :limit)
    end
  2. Explicitly mark attributes optional:

    class Filter
      include Value.new(:matcher, :limit)
      optional_attributes :limit # default to nil
      # or provide an explicit default
      optional_attributes limit: Float::INFINITY
      # or mark multiple attributes as optional
      optional_attributes matcher: //, limit: Float::INFINITY
    end
  3. Accept an options hash in the constructor

    class Filter
      include Value.new(:matcher, :limit, defaults: {limit: Float::Infinity})
    end
  4. Don't support this feature in the gem; require users to override .with to provide defaults:

    class Filter
      include Value.new(:matcher, :limit)
    
      ATTRIBUTE_DEFAULTS = {limit: Float::Infinity}
    
      def self.with(hash)
        attributes = hash.dup
        attributes[:limit] ||= nil
        super(ATTRIBUTE_DEFAULTS.merge(hash))
      end
    end

@tcrayford Could you please comment on whether you like these changes and your opinion on the potential approaches?

/cc @josephjaber

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