avdi / naught

A toolkit for building Null Object classes in Ruby
MIT License
1.05k stars 53 forks source link

Delegate explicit conversions to nil instead of defining them explicitly #44

Closed sferik closed 10 years ago

sferik commented 10 years ago

This refactoring:

  1. reduces the number of lines in DefineExplicitConversions#call (11.downto(6)),
  2. removes 7 single-line method definitions, which are ugly IMHO (i.e., it’s actually a 25-line method, minified),
  3. makes the code easier to modify (if new conversion methods are added to NilClass in future Ruby versions),
  4. works more consistently across different Ruby versions, and most importantly,
  5. is more intention-revealing (do what nil does vs. do this explicit thing, which happens to be what nil does).

I've also taken this opportunity to alphabetize the method names, for organizational purposes.

coveralls commented 10 years ago

Coverage Status

Coverage remained the same when pulling 6743cdb7972ed9860f171460d8a4f2d5e701ffbe on sferik:delegate_explicit_conversions into 7423689dc34e1e3bcdbf15a4d920fbe7136ab1da on avdi:master.

coveralls commented 10 years ago

Coverage Status

Coverage decreased (-0.0%) when pulling 720d853d221e42c354c8799aa8bcfafad18c36e0 on sferik:delegate_explicit_conversions into 7423689dc34e1e3bcdbf15a4d920fbe7136ab1da on avdi:master.

coveralls commented 10 years ago

Coverage Status

Coverage decreased (-0.0%) when pulling 85c195de80ed56993b88f47e09112c903a92a167 on sferik:delegate_explicit_conversions into 7423689dc34e1e3bcdbf15a4d920fbe7136ab1da on avdi:master.

avdi commented 10 years ago

Very cool.

sferik commented 10 years ago

Thanks for reviewing and merging this patch.

Upon re-reading my description above, I wanted to clarify one of the points:

works more consistently across different Ruby versions

What I mean by this is: the behavior of naught will be more consistent with the Ruby environment in which it is running. I realized this sentence could be construed to mean the opposite: that naught will behave identically across Ruby versions. This is not the case.

For example, NilClass#to_h was only added Ruby 2.0 (after I suggested it back in 2009). Before this patch was applied, null objects with explicit conversions would define a to_h method, even on Ruby 1.9, where nil does not respond to to_h. This patch ensures that null objects with explicit conversions will respond exactly like nil on whatever Ruby environment you happen to be running.

This could result in backward-incompatibility for programs that depend on null objects responding to to_h with {} on Ruby 1.9. Please consider this when choosing a version number for the next release, although Semantic Versioning does not have strong prescriptions for gems that have not reached 1.0.0.

Arguably, it was a bug that null objects ever responded to to_h on Ruby 1.9 but if you want to add back this feature, you could do as a one-off (i.e. delegate everything expect for to_h on Ruby 1.9). The same thing could be done for to_c and to_r on Ruby 1.8, if I ever figure out how to make the specs pass. That said, in my opinion, such steps are unnecessary and the post-merge behavior is how it should have always worked.

avdi commented 10 years ago

Someone yelled at me for the sub-1.0 version once, but this is exactly why! These things still needed some peer-review... I'm fine with this being the new normal; it's easy enough to override the definitions in per-project Naught configurations.