Refactor request param parsing into configurable ParamParser objects. #15

Merged
merged 5 commits into from Oct 23, 2012

Projects

None yet

3 participants

@myronmarston
Member

@benkirzhner / @proby -- can you guys code review this?

I still need to update Vanguard to use this for array types but I think this should be all we need for that.

@proby proby and 1 other commented on an outdated diff Oct 23, 2012
lib/interpol/configuration.rb
+ param.string_validation_options 'pattern' => '^\-?\d+(\.\d+)?$'
+
+ param.parse do |value|
+ begin
+ Float(value)
+ rescue TypeError
+ raise ArgumentError, "Could not convert #{value.inspect} to a float"
+ end
+ end
+ end
+
+ define_request_param_parser('boolean') do |param|
+ param.string_validation_options 'enum' => %w[ true false ]
+
+ booleans = { 'true' => true, true => true,
+ 'false' => false, false => false }
@proby
proby Oct 23, 2012

Are 'TRUE', 'FALSE', or other string variants valid? I just did a quick look at the JSON schema docs and I couldn't find a definitive answer

@myronmarston
myronmarston Oct 23, 2012 Member

JSON doesn't allow string "true" and "false" to stand in for true and false--so no, the capitolized variants aren't valid, but neither are these. We need to allow strings because that's what a params hash gives us. We can decide what variants we allow. I like keeping it pretty strict and just using the lowercase strings.

@proby
proby Oct 23, 2012

Just checking, I agree with keeping this as strict as possible. Which actually leads me to a separate thought. Can someone override these built in parsers by redefining them?

@myronmarston
myronmarston Oct 23, 2012 Member

Yep, see this spec. That's the reason I used unshift (rather than <<) so that parsers defined later take precedence.

@proby
proby commented Oct 23, 2012

Good changes.

@benkirzhner benkirzhner and 1 other commented on an outdated diff Oct 23, 2012
lib/interpol/configuration.rb
@@ -250,6 +272,141 @@ def register_default_callbacks
select_example_response do |endpoint_def, _|
endpoint_def.examples.first
end
+
+ register_built_in_param_parsers
+ end
+
+ def register_built_in_param_parsers
@benkirzhner
benkirzhner Oct 23, 2012 Member

This file is getting pretty large. For clarity, it might be worth splitting out this method and register_default_callbacks into their own files/classes. Perhaps configuration/{built_in_param_parsers,default_callbacks}.rb?

@myronmarston
myronmarston Oct 23, 2012 Member

That's a good idea.

@benkirzhner
Member

Ship it!

@myronmarston myronmarston merged commit e6b32bf into master Oct 23, 2012

1 check passed

default The Travis build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment