[WIP] Add nested get query parameter expandation #84
Conversation
mustermann/mustermann.gemspec
Outdated
| @@ -14,4 +14,6 @@ Gem::Specification.new do |s| | |||
| s.files = `git ls-files`.split("\n") | |||
| s.test_files = `git ls-files -- {test,spec,features}/*`.split("\n") | |||
| s.executables = `git ls-files -- bin/*`.split("\n").map{ |f| File.basename(f) } | |||
|
|
|||
| s.add_dependency 'query_string_builder' | |||
There was a problem hiding this comment.
I can remove this dependency, it is my sample code
|
inlin-ized and fixup this commit 1e5a12d |
There was a problem hiding this comment.
Did you read the source code?
Did you dig into the problem to understand the essence?
Your patch breaks expanding features.
By executing the following code, you can check the differences before and after applying your changes.
require 'mustermann'
pattern = Mustermann.new("/books/:id")
p pattern.expand(:append, id: "foo", bar: "&#?")| @@ -194,8 +194,7 @@ def slice(hash, keys) | |||
|
|
|||
| def append(uri, values) | |||
| return uri unless values and values.any? | |||
| entries = values.map { |pair| pair.map { |e| @api_expander.escape(e, also_escape: /[\/\?#\&\=%]/) }.join(?=) } | |||
| "#{ uri }#{ uri[??]??&:?? }#{ entries.join(?&) }" | |||
| [uri, QueryStringBuilder.new(values).build].join('?') | |||
There was a problem hiding this comment.
I don't think QueryStringBuilder is necessary. Recursive processing looks fine, but QueryStringBuilder is really overkill.
| @@ -194,8 +194,7 @@ def slice(hash, keys) | |||
|
|
|||
| def append(uri, values) | |||
| return uri unless values and values.any? | |||
| entries = values.map { |pair| pair.map { |e| @api_expander.escape(e, also_escape: /[\/\?#\&\=%]/) }.join(?=) } | |||
| "#{ uri }#{ uri[??]??&:?? }#{ entries.join(?&) }" | |||
There was a problem hiding this comment.
Why did you delete these lines? Your patch doesn't cover these feature.
mustermann/spec/expander_spec.rb
Outdated
| @@ -70,6 +70,9 @@ | |||
| subject(:expander) { Mustermann::Expander.new('/:a', additional_values: :append) } | |||
| example { expander.expand(a: ?a).should be == '/a' } | |||
| example { expander.expand(a: ?a, b: ?b).should be == '/a?b=b' } | |||
| example { expander.expand(a: ?a, b: [?b]).should be == '/a?b[]=b' } | |||
| example { expander.expand(a: ?a, b: {c: ?c}).should be == '/a?b[c]=c' } | |||
| example { expander.expand(a: ?a, b: {c: [1, 2]}).should be == '/a?b[c][]=1&b[c][]=2' } | |||
There was a problem hiding this comment.
The original test doesn't mention escape, but you need to add about it at least to add this change.
iguchi1124
changed the title
|
Thank you for reviews 😁I'll update this pull request soon. |
|
@iguchi1124 If you're going to include this fix in the 1.0.3 release, please write to that effect in the milestone issue. |
iguchi1124
changed the title
iguchi1124
changed the title
| @@ -2,6 +2,7 @@ | |||
| require 'mustermann/ast/expander' | |||
| require 'mustermann/caster' | |||
| require 'mustermann' | |||
| require 'addressable/uri' | |||
There was a problem hiding this comment.
What is this?
I would not like to use the addressable gem. Mustermann has not been depended on external gems.
Is there a clear reason to add this gem?
If so, why do not you add this to gemspec.
|
@iguchi1124 Hey, I'm going to release new version next week. So if you'd like to include this fix at the next version, please ping me. |
I'm creating this issue #61 examples in this pull request!
What do you think about this approach?