Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

mo fasta and mo betta the url_for #5750

Merged
merged 2 commits into from
Apr 5, 2012
Merged

mo fasta and mo betta the url_for #5750

merged 2 commits into from
Apr 5, 2012

Conversation

ahoward
Copy link
Contributor

@ahoward ahoward commented Apr 5, 2012

incorporated @tenderlove's suggestions.

@carlosantoniodasilva
Copy link
Member

@ahoward great! Just a note, it's always good if you can link to the other related issue when sending a new issue / pull request, so others who didn't see it before can follow what's going on. Thanks :)

Update: related to #5743.

_routes.url_for(url_options)
when Hash
symbolized = {}
options.keys.each do |k|
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we create a private method to do this?

Something like this:

def merge_hash(target, base)
  base.keys.each do |k|
    sym = k.to_sym
    target[sym] = base[k] unless target.has_key?(sym)
  end

  target
end

and call it like this:

symbolized = {}
merge_hash(symbolized, options)
merge_hash(symbolized, url_options)
_routes.url_for(symbolized)

What did you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i did, but if you read the previous discussion people are worried about 1.0 vs 0.4 seconds for 1_000_000 iterations. adding a function call won't help you there. personally i would, but it seems contrary to the goal of making it as fast as possible. again, i'd personally go for well factored code and let the byte code writers get busy.... /cc @tenderlove

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rafaelfranca this also makes the same bad assumption the current does: that target is symbolized. the bugz being caused can be summarized as

"we can't know if options or url_options has had string or symbol keys written into it"

tenderlove added a commit that referenced this pull request Apr 5, 2012
mo fasta and mo betta the url_for
@tenderlove tenderlove merged commit 174cf8b into rails:master Apr 5, 2012
spastorino added a commit that referenced this pull request Apr 9, 2012
This reverts commit 174cf8b, reversing
changes made to 7ecd6a7.
The reverted commit improved the performance in the wrong place, now we
have added this 6ddbd18 improvement.
nordringrayhide pushed a commit to nordringrayhide/rails that referenced this pull request May 2, 2012
This reverts commit 174cf8b, reversing
changes made to 7ecd6a7.
The reverted commit improved the performance in the wrong place, now we
have added this 6ddbd18 improvement.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants