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

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

Merged
merged 3 commits into from
Jan 6, 2014
Merged

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

merged 3 commits into from
Jan 6, 2014

Conversation

sferik
Copy link
Collaborator

@sferik sferik commented Jan 5, 2014

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
Copy link

Coverage Status

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

@coveralls
Copy link

Coverage Status

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

Bundler's Gemfile gives you the ability to differentiate between
development dependencies and test dependencies. Non-test development
dependencies (e.g. guard) do not need to be installed in the continuous
integration environment, making the build somewhat faster.

This leaves bundler as the only dependency specified in the gemspec,
since it is needed to bootstrap dependencies specified in the Gemfile.
@coveralls
Copy link

Coverage Status

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

@avdi
Copy link
Owner

avdi commented Jan 6, 2014

Very cool.

avdi pushed a commit that referenced this pull request Jan 6, 2014
Delegate explicit conversions to nil instead of defining them explicitly
@avdi avdi merged commit 3ad13c6 into avdi:master Jan 6, 2014
@sferik sferik deleted the delegate_explicit_conversions branch January 6, 2014 19:11
@sferik
Copy link
Collaborator Author

sferik commented Jan 6, 2014

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
Copy link
Owner

avdi commented Jan 6, 2014

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.

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