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

RouteSet: decomplecting a way to handle positional args #5957

Merged
merged 1 commit into from Apr 24, 2012

Conversation

Projects
None yet
3 participants
Contributor

bogdan commented Apr 24, 2012

Currently positional arguments are handle with a nasty hack: bypassing them though options hash.
This make code wider and harder to understand.

This patch merges it all together and removes options[:_positional_args] and options[:_positional_keys] hack.

Ensure no performance degrade:

https://gist.github.com/2479082


$ diffbench perf.rb
Running benchmark with current working tree
Stashing changes
Running benchmark with clean working tree
Applying stashed changes back

                    user     system      total        real
----------------------------------Url helper module builder
After patch:    0.410000   0.000000   0.410000 (  0.409276)
Before patch:   0.410000   0.000000   0.410000 (  0.413838)

--------------------------------------simple URL generation
After patch:    0.010000   0.000000   0.010000 (  0.005349)
Before patch:   0.000000   0.000000   0.000000 (  0.005511)

--------------------URL generation with optional parameters
After patch:    0.090000   0.000000   0.090000 (  0.091728)
Before patch:   0.100000   0.000000   0.100000 (  0.092634)

@josevalim josevalim added a commit that referenced this pull request Apr 24, 2012

@josevalim josevalim Merge pull request #5957 from bogdan/routes
RouteSet: decomplecting a way to handle positional args
afcae34

@josevalim josevalim merged commit afcae34 into rails:master Apr 24, 2012

@carlosantoniodasilva carlosantoniodasilva commented on the diff Apr 24, 2012

actionpack/lib/action_dispatch/routing/route_set.rb
@@ -96,7 +96,24 @@ class NamedRouteCollection #:nodoc:
def initialize
@routes = {}
@helpers = []
- @module = Module.new
+ @module = Module.new do
+ protected
+ def handle_positional_args(args, options, route)
+ inner_options = args.extract_options!
+ result = options.dup
+
+ if args.any?
+ keys = route.segment_keys
+ if args.size < keys.size - 1 # take format into account
+ keys -= self.url_options.keys if self.respond_to?(:url_options)
+ keys -= options.keys
+ end
+ result.merge!(Hash[args.zip(keys).map { |v, k| [k, v] }])
@carlosantoniodasilva

carlosantoniodasilva Apr 24, 2012

Owner

Can this be handle by Hash#invert?

# before
Hash[args.zip(keys).map { |v, k| [k, v] }]
# after
Hash[args.zip(keys)].invert

Wdyt?

@bogdan

bogdan Apr 24, 2012

Contributor

I was just copy pasting that line,
You can check it on your side to be sure.

@carlosantoniodasilva

carlosantoniodasilva Apr 24, 2012

Owner

Yeah I see that they are the same lines. I'll check later, thanks.

@josevalim

josevalim Apr 24, 2012

Contributor

Couldn't we simply do? keys.zip(args)

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