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

Add to_enum to the list of defined explicit conversions #46

Closed
wants to merge 7 commits into from
Closed

Add to_enum to the list of defined explicit conversions #46

wants to merge 7 commits into from

Conversation

sferik
Copy link
Collaborator

@sferik sferik commented Jan 7, 2014

I was curious if there were any other conversion methods defined on nil that were not being delegated in Naught::NullClassBuilder::Commands::DefineExplicitConversions, so I ran the following command:

> nil.methods.select{|method| method.to_s =~ /^to_\w+$/}
=> [:to_i, :to_f, :to_s, :to_a, :to_h, :to_r, :to_c, :to_enum]

Currently, DefineExplicitConversions defines all of these methods save for one: to_enum. I’m not sure how useful this will be in practice, but it seems like the right thing to do, for the sake of completeness. And—who knows—maybe someone out there wants to use a null object to stand in for an Enumerator. It’s not that crazy.

Of course, if you try passing a block to any Enumerable method on the null Enumerator, it will fail with the NoMethodError:

undefined method `each' for nil:NilClass

I debated whether the null enumerator should be defined as [].to_enum or {}.to_enum, so it would iterate zero times instead of raising an error, but ultimately settled on nil.to_enum, mostly for consistency in both the specs and implementation.

This patch does a few other things as well. It:

  1. renames and improves the explicit conversions spec, to better match the new implementation,
  2. runs the specs in random order to catch order-dependency bugs, and
  3. makes the whitespace in this project more consistent.

In case you’re curious, I use the following command to do that, which I keep as an alias:

find . -not \( -name .svn -prune -o -name .git -prune \) -type f -print0 | xargs -0 sed -i "" -E "s/[[:space:]]*\$//"

Rename for consistency and clarity vs. implicit_conversions_spec.rb.
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.0%) when pulling f16ec06 on sferik:to_enum into 3ad13c6 on avdi:master.

@sferik
Copy link
Collaborator Author

sferik commented Jan 7, 2014

Hmmm, it looks like requiring coveralls in the Travis environment is causing the specs to fail (they pass locally) because it depends on rest-client, which adds the following methods to NilClass, for which there are not delegates:

  • #to_json
  • #to_yaml
  • #to_yaml_properties

That got me thinking about a different implementation that uses method_missing for delegation. That way, if NilClass responds to to_json at runtime, it will automatically delegate to that method. This seems like an even better approach that listing out all the delegate methods manually.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.16%) when pulling 42515c9 on sferik:to_enum into 3ad13c6 on avdi:master.

@sferik
Copy link
Collaborator Author

sferik commented Jan 7, 2014

Insofar as one of the goals of this library is to have fun with use metaprogramming, this patch definitely furthers this goal. 😏

Specs are passing now, including on Ruby 1.9.3, which was failing before this patch.

@avdi
Copy link
Owner

avdi commented Jan 7, 2014

We need to be very careful here that there's no chance of interfering badly with any other places that #method_missing may be defined in the process of building a null class. In particular, how does this play with black_hole?

This change to the specs more accurately reflects the implementation,
which now delegates all of these methods to nil. This change also allows
the specs to run (and pass) on Ruby 1.9 without any conditional logic.
@coveralls
Copy link

Coverage Status

Coverage decreased (-0.0%) when pulling 1bdd59d on sferik:to_enum into 3ad13c6 on avdi:master.

@sferik
Copy link
Collaborator Author

sferik commented Jan 7, 2014

@avdi Ah, I hadn’t considered how using method_missing there would interact with other commands. Perhaps there should be an integration test that includes all the commands and specifies the behavior of the combined null object?

In the mean time, I’ve rolled back to the previous implementation, that used method_missing. I’ve also added pry as a development dependency, which was helpful in debugging this issue.

Feel free to cherry-pick only the commits you like.

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.0%) when pulling d3ba865 on sferik:to_enum into 3ad13c6 on avdi:master.

@avdi
Copy link
Owner

avdi commented Jan 8, 2014

Most of these changes are no-brainer accepts. I'm still a little dubious about the central one though. #to_enum isn't really a NilClass thing AFAIK; it's on nil only because NilClass < Object. If users specify config.base = Object they'll get #to_enum; otherwise they won't.

I'm not sure how common use of to_enum is, but I feel like if we're going to add it to the explicit conversions it ought to return something "null-ish" rather than something that doesn't work.

I guess the central question is: given I have a NullObject standing in for some_collection--assuming there is a reason to do this rather than use an plain old empty array--what does the POLS suggest some_collection.to_enum would return? What about some_collection.to_enum(:some_method)?

My knee-jerk response to this is that if we have it, some_collection.to_enum should be the equivalent of some_collection.to_a.to_enum. Thoughts?

...and sorry to be so difficult!

@sferik
Copy link
Collaborator Author

sferik commented Jan 8, 2014

I don’t think you’re being difficult at all. You’re asking the questions that a project maintainer should ask. As I mentioned above, I asked these very same questions to myself:

I debated whether the null enumerator should be defined as [].to_enum or {}.to_enum, so it would iterate zero times instead of raising an error, but ultimately settled on nil.to_enum, mostly for consistency in both the specs and implementation.

This may have been the wrong decision on my part. I don’t feel confident enough in the decision to really defend it and I’m growing dubious of the usefulness of this feature, so I’m pretty happy to close it at this point. I’ve already pull the other commits into my ruby_1.8 branch, so those will not be lost.

Thanks for considering this.

@sferik sferik closed this Jan 8, 2014
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