Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Default options #91

Merged
merged 5 commits into from Feb 18, 2013

Conversation

Projects
None yet
2 participants
Member

rwz commented Feb 15, 2013

Changes so far:

  • MultiJson module has separate load_options and dump_options getter/setter
  • default_options is deprecared and sets both dump_options and load_options
  • Options accept hashes, lambdas and to_hash-objects
  • Each adapter supports it's own load_options and dump_options getters/setters and it's priority is higher than global

@sferik sferik commented on an outdated diff Feb 16, 2013

lib/multi_json.rb
@@ -107,8 +115,8 @@ def load(string, options={})
# :nodoc:
alias :decode :load
- def current_adapter(options)
- if new_adapter = (options || {}).delete(:adapter)
+ def current_adapter(options={})
+ if new_adapter = options.delete(:adapter)
@sferik

sferik Feb 16, 2013

Member

This is a nice little change.

@sferik sferik commented on an outdated diff Feb 16, 2013

lib/multi_json.rb
@@ -11,9 +14,14 @@ def initialize(message='', backtrace=[], data='')
end
DecodeError = LoadError # Legacy support
+ def default_options=(value)
+ Kernel.warn 'MultiJson.default_options setter is deprecated\n' +
+ 'Use MultiJson.load_options and MultiJson.dump_options instead'
+
+ @dump_options = value
@sferik

sferik Feb 16, 2013

Member

I still don’t understand why you’re setting @dump_options but not @load_options here.

@sferik sferik commented on an outdated diff Feb 16, 2013

lib/multi_json.rb
- @default_options = {}
- attr_accessor :default_options
+ alias :default_options :dump_options
@sferik

sferik Feb 16, 2013

Member

Where is the dump_options accessor defined? Shouldn’t there be a load_options accessor too?

I'm not sure how I feel about aliasing default_options to dump_options, even if they were the same in the past.

@sferik sferik commented on the diff Feb 16, 2013

lib/multi_json.rb
@@ -117,9 +125,7 @@ def current_adapter(options)
# Encodes a Ruby object as JSON.
def dump(object, options={})
- options = default_options.merge(options)
- adapter = current_adapter(options)
- adapter.dump(object, options)
+ current_adapter(options).dump(object, options)
@sferik

sferik Feb 16, 2013

Member

This is much cleaner. Very nice.

@sferik sferik commented on an outdated diff Feb 16, 2013

lib/multi_json/options.rb
@@ -0,0 +1,38 @@
+module MultiJson
+ module Options
+ attr_writer :load_options, :dump_options
+ def load_options(*args)
+ get_options :load_options, *args
+ end
+
+ def dump_options(*args)
+ get_options :dump_options, *args
+ end
+
+ def default_load_options
+ {}
@sferik

sferik Feb 16, 2013

Member

Please make this a constant or memoize it.

@sferik sferik commented on an outdated diff Feb 16, 2013

lib/multi_json/options.rb
+ module Options
+ attr_writer :load_options, :dump_options
+ def load_options(*args)
+ get_options :load_options, *args
+ end
+
+ def dump_options(*args)
+ get_options :dump_options, *args
+ end
+
+ def default_load_options
+ {}
+ end
+
+ def default_dump_options
+ {}
@sferik

sferik Feb 16, 2013

Member

Ditto.

Member

sferik commented Feb 16, 2013

There's something wrong with your tests. They seem to be failing randomly on Travis. Perhaps there’s an order-dependency issue?

Member

rwz commented Feb 17, 2013

Fixed tests, cleaned the code, cached default options.

Member

rwz commented Feb 17, 2013

I've tried replacing default_{load|dump}_options with instance variables, but that did work with inheritance.

@sferik sferik commented on an outdated diff Feb 17, 2013

lib/multi_json.rb
@@ -11,9 +14,14 @@ def initialize(message='', backtrace=[], data='')
end
DecodeError = LoadError # Legacy support
+ alias :default_options :dump_options
@sferik

sferik Feb 17, 2013

Member

Can you at least add a comment here explaining the historical reasons why this alias makes sense? I think someone reading the code for the first time would be confused by this.

Member

sferik commented Feb 17, 2013

Overall, this looks good to me. Once you add a comment around above line 17 of multi_json.rb, I think it’s good to merge. 👍

Member

rwz commented Feb 18, 2013

Added comment explaining default_options alias

@sferik sferik added a commit that referenced this pull request Feb 18, 2013

@sferik sferik Merge pull request #91 from intridea/default-options
Default options
8f3b7b6

@sferik sferik merged commit 8f3b7b6 into master Feb 18, 2013

1 check passed

default The Travis build passed
Details

@mshytikov mshytikov referenced this pull request Feb 21, 2013

Closed

gem v 1.6.1 broke my app #92

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment