search

avdi / naught

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

Fix serialization of black_hole to_yaml #67

Closed bf4 closed 9 years ago

bf4 commented 9 years ago

Fixes #66 See there for details

bf4 commented 9 years ago

I'm not sure what the ideal return value for #to_int is, but nil doesn't work and 0 doesn't seem to have any ill-effects.

@tenderlove any thoughts on the meaning of define_method(:to_int) { 0 } on serialization of {null: null}.to_yaml?

Ref: Psych::Visitors::YAMLTree

    def key? target
      @obj_to_node.key? target.object_id
    rescue NoMethodError
      false
    end

should psych also rescue a TypeError, instead of just NoMethodError?

I may also open this as a bug in ruby/psych that it only rescues a NoMethodError, not TypeError.. we'll see

bf4 commented 9 years ago

failures are rubocop..

sferik commented 9 years ago

If you rebase from master, the RuboCop failures should be fixed. While you’re rebasing, would you mind doing an interactive rebasing and squashing these changes into a single commit? Ideally, the tests and related code changes would be in a single, atomic commit, in case they need to be reverted.

I have mixed feelings about the content of this change. If seems like quite a dirty hack. I’ll leave it up to @avdi as to whether this should be merged or not. Perhaps this problem is better addressed in the twitter gem directly than at the library level?

bf4 commented 9 years ago

@sferik in the meantime, would you like a PR to add to_int to the Twitter::NullObject? That was my original patch before I dug down into it.

sferik commented 9 years ago

@bf4 I would like @avdi to weigh-in first. If it’s an urgent issue for you, feel free to monkey patch your application.

bf4 commented 9 years ago

@sferik not urgent at all, in fact my Gemfile is now

# until https://github.com/avdi/naught/pull/67 is merged
gem 'naught', github: 'bf4/naught', branch: 'bf/fix_black_hole'
gem 'twitter'
sferik commented 9 years ago

Fixed in https://github.com/avdi/naught/commit/d32d4ea32a9a847bffd6cf18f480bdfaaf7a3641.

bf4 commented 9 years ago

@sferik Thanks for taking care of this. I guess it couldn't be merged anymore?

sferik commented 9 years ago

I took a different approach to solving this problem, defining to_int as an implicit conversion.

bf4 commented 9 years ago

:smile:

sferik commented 9 years ago

Released in version 1.1.0.