Hi @tcrayford,

Just like the issue for Travis CI, I just need you to sign up and enable this repo so that I can add the configuration and the badge.

https://codecov.io/

Thanks,
-Marc

Hi,

Here is an example of "mutability" over the attributes.

[1] pry(main)> X = Value.new(:x)
=> X
[2] pry(main)> x = X.new(%w[1 2 3])
=> #<X x=["1", "2", "3"]>
[3] pry(main)> x.x.concat %w[4 5 6]
=> ["1", "2", "3", "4", "5", "6"]
[4] pry(main)> x.x
=> ["1", "2", "3", "4", "5", "6"]

[EDIT] replaced ValueObject by Value in code snippet

Not sure this is wanted but it might be confusing as we could think that the library would take care of such case.

What do you think?

cc @ignacykasperowicz @mpapis

I notice that although .new validates the presence of all arguments, .with doesn't validate that all keys are provided (and defaults the attributes to nil). Is this intended behaviour? Is there are reason this is preferable? Seems like it would be easy to be tripped up by not specifying a value to .with.

If this behaviour is desirable, I'm happy to submit a PR

Would it be good to have a method that lets you provide a block to "modify" a particular attribute (returning a copy of course)?

E.g., either my_thing.update(:attr1){|attr1| ...} or my_thing.with(:attr1){|attr1|...} or maybe somehow a special method for each attribute?

Problem

The Values gem in Ruby, much like case classes in Scala, makes a very sensible and pragmatic trade-off in its approach to enforcing immutability:

• Value class instances are always frozen after construction
• Actual values wrapped by the instances are not required to be frozen, however

Note: I 100% agree with this trade-off for a variety of reasons, no need to discuss here.

However, the current implementation has a problem in the case that a mutable wrapped value, such as an Array or String, is mutated after being wrapped by a value object. In such cases, the value object will have inconsistent #hash and #eql? methods, which violates Ruby language specification:

This function must have the property that a.eql?(b) implies a.hash == b.hash.

Example

Stats = Value.new(:totals)

x = Stats.new([1])
y = Stats.new([1])

[x == y, x.hash == y.hash]
#=> [true, true]

#
# EDGE CASE: mutate value of attribute of y
#
y.totals << 2
y
#=> #<Stats totals=[1, 2]>

[x == y, x.hash == y.hash]
#=> [false, true] <-- INCONSISTENCY

Analysis

As opposed to case classes in Scala, the Values gem currently pre-calculates the hash codes in all instance constructors. This enforces paying the cost of a call to #hash exactly once per instance, as a trade-off against paying it when used as a Hash key or as a Set element.

Because it is pre-calculated in all instances it cannot take mutations into account, but #eql? does take mutations into account.

Proposal

I'd already noticed that when wrapping sufficiently complex values, the time it takes to pre-calculate the hash code is already a significant drag on performance due to performing an expensive calculation during tight loops of functional code that produce an intermediate value object in each iteration. It appears that we could improve this gem, as well as fix this behavior, by introducing an opt-in way to pre-calculate certain methods in general, and then treat the #hash method as just one case of the more general phenomenon of wanting a "memoize this method" solution for immutable value objects.

Then, the general case could be that hash is no longer pre-calculated, speeding up general usage of Value objects when they are not to be used often as Hash keys or in Sets.

And in the cases where we do want the hash to be pre-calculated, we could use machinery such as:

# Returns a single instance with `#hash` pre-calculated
x2 = x.with_precalculated(:hash)

[x2 == x, x2.hash == x.hash]
#=> [true, true]

# Or, set a method as always pre-calculated at class level
Stats = Value.new(:totals) do
  precalculate :hash
end

This is an RFC, please review @tcrayford @michaeldiscala and other interested folks!

The #to_a method implemented by the Values gem, while sensible in isolation, is actively harmful in practice.

Why sensible in isolation?
Because in some sense, a value object is nothing more than a tuple of values. Just as, intuitively, #to_h converts the named fields to a hash of names to values, it is intuitive that #to_a have a similar meaning.

Why harmful in practice?
It turns out that #to_a is a bit of a privileged operation in Ruby. There are a number of important scenarios in Ruby code where this method, if it is implemented, is called automatically, leading to results that are not what the implementor intended. For example:

Point = Value.new(:x, :y)

p = Point.new(1, 2)

# Wrap point in an Array - expected: [#<Point x=1, y=2>]
Array(p)

# Nope! Oops, it implicitly calls #to_a
#=> [[:x, 1], [:y, 2]]

# Similarly, use splat args where the input may be single value OR Array
a = *2
#=> [2] <-- expected behavior

b = *p
#=> [[:x, 1], [:y, 2]] <-- unexpected behavior! expected [#<Point x=1, y=2>]

For a similar view explained differently, see also You should not implement #to_a for your classes.

For evidence of Matz's philosophy on what #to_a means, specifically: I consider
"*x" as a form of explicit conversion, i.e. shorthand for "(x.to_a)", see Ruby bug 1393, comment 5.

Hi @tcrayford, thanks for this great library. I like the design choices that you made here.

I'd like to use Values as a basis for a re-writing of the Rumonade gem. However, before doing so there's a series of changes I'd like to propose and discuss with you, before submitting all the PRs:

1. Fix zero-field Value class edge case (#23)
2. Fix Gemfile.lock issue (#22)
3. Add a #with method to support hash value replacement (#24)
4. Add a #to_h method to support conversion to Hash like OpenStruct (#27)
5. Add Travis CI and badge (#28)
6. Add CodeCov.io and badge (#29, #36 )
7. Add gem version badge (#30)
8. Document with YARD and add a rubydoc.info link to docs (#38)
9. Document that it also replaces OpenStruct (via #with and #to_h) (#39)
10. Release a new major version including all above changes (#40)
11. On supported Ruby versions (2+), use keyword args in .with and #with
12. Add support for default and required values in .with
13. Release a new major version including above features

Thanks again for this library and your time. My only interest in making these changes is to get the library to the point where I can use it as the basis for all immutable types in next version of Rumonade :)

(this issue edited to reflect discussion)

To me using freeze is enforcing a certain implementation of immutability. Without freeze I can easily create idiomatic memoized accessors on the value for derived fields:

def ratio
    @ratio ||= something / another
end

With freeze I have to resort to ugliness. Since @vars are internal anyway I'd argue it's more rubyish to keep the external API immutable, while allowing mutation internally to implement that external immutable API efficiently.

Using this gem, I've noticed implicit type coercion (via to_hash) exposing the hash attribute in lots of places (e.g. through ActiveSupport's as_json. From reading https://zverok.github.io/blog/2016-01-18-implicit-vs-expicit.html I'd guess that this Library should impliment to_hash

Curious if there's any objection, else will open a PR.

In the mean time, here's a hacky monkeypatch:

https://gist.github.com/michaelglass/b702603f4d02ffbd16cfc884896e44dd

I suggest that there should be a documented best practice for documenting value classes with yard.

Requirements:

• Works on rubydoc.info for docs
• Works in Rubymine for completion
• Allows documenting the type(s) of each field

I've put a gist together showing how it currently works. As you can see, Example3 works about 75%, which I think is the best we can do at the moment.

So that leads me to a documentation proposal. What do you think of this:

How to Document a Value class using YARD

Current best practice for writing YARD documentation for value classes created with this gem can be best summed up as "make them look like Structs for YARD":

1. Use the inheritance syntax (eg class A < Value.new(...)) rather than assigning to a constant
2. Document each field using YARD tag @attr_reader (docs)

Example

# What is a person?
#
# @attr_reader [String]        name     Full name
# @attr_reader [Fixnum]        age      Age in years
# @attr_reader [Array<Person>] friends  List of friends
class Person < Value.new(:name, :age, :friends); end

Still TODO

• tell YARD about generated constructor
• tell YARD about generated class methods: .with
• tell YARD about the generated instance methods: #==, #eql?, #values, #inspect, #with, #to_h, #to_a

Currently, the implementation of Value#inspect is always used, preventing the PP system from indenting large value classes

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)
       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

Hi @tcrayford, sorry to bother you.

Before I can enable Travis CI, configure the build, add the badge, etc. You must sign into Travis CI and enable this repo. So I've created this issue to track that.

Regards,
-Marc

Per HISTORY.md, v1.9.0 was released on Aug 20, 2016. However, there is no existing tag for 1.9 and gem is current installing 1.8.

Please publish the 1.9 gem. In my personal use case, I would like to make use of the new recursive_to_h method for JSON serialization.

Thanks!

Was wondering if you're close to releasing a new version to rubbygems. I keep finding myself needing the method customization feature and it not being available on the published version.

Hi Tom,

I'm sorry to use this issue tracker for this, but I didn't find a better way to ask about this in an open way.

I've found out about Values in Michael Fairley's slide deck Immutable Ruby.

I've been using Virtus::ValueObject (provided by Virtus) for a few months and I'm really curious about the differences.

Thanks for your answer and for making open-source stuff.

I've been using a toooooooon of nested Value types lately. They rule. The only downside is writing a bunch of custom to_h methods so I can serialize them to JSON.

Address = Value.new(:street, :zip)
Value.new(:name, :address)
User.with(name: 'blinsay', address: Address.with(street: 'the internet', zip: '12345')).to_h
# => {:name=>"blinsay", :address=>#<Address street="the internet", zip="12345">}

I've been working around this by doing something like the following, but it sucks to have to do this in every new project:

module RecursiveToHash
  def to_h
    pairs = to_a.map do |k, v|
      if v.respond_to?(:to_h)
        [k, v.to_h]
      else
        [k, v]
      end
    end

    pairs.to_h
  end
end

class Value
  def self.create(*fields, &block)
    new(*fields, &block).tap do |klass|
      klass.prepend(RecursiveToHash)
    end
  end
end

Thoughts on making to_h recursive by default, or adding a to_h_recursive method?

Using values as hash keys takes a pretty big performance hit because the hash is constantly recomputed. Normally we'd just say @hash ||= ... in the hash method, but that would violate the freeze. (I'd rather have freeze than ||= in this case.)

How about precomputing the hash at construction time? Something like: https://gist.github.com/garybernhardt/5863735 ? In my particular system, this drops runtime to 85% of what it was before.

As an aside, I see no noticeable performance hit for replacing the existing hash with:

        @hash = self.class.hash ^ values.hash

which will incur almost no performance hit at initialization time.