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

Eliminate extra template lookup in ActionView::Digestor #25826

Merged
merged 1 commit into from Jul 15, 2016

Conversation

Projects
None yet
3 participants
@javan
Member

javan commented Jul 14, 2016

PathSet#exists? and PathSet#find both do the same thing (PathSet#find_all). The exists? avoids MissingTemplate errors when no template is found. This change avoids it by using find_all.first instead. No behavior change, just one less template lookup.

Noticed this while working on #25411.

/cc @dhh @matthewd

@javan javan added the actionview label Jul 14, 2016

@dhh

This comment has been minimized.

Member

dhh commented Jul 15, 2016

Sounds good to me. Any concerns @rafaelfranca ?

@rafaelfranca

This comment has been minimized.

Member

rafaelfranca commented Jul 15, 2016

:shipit: . We may want to change the API of PathSet to have a find and find! method so we don't raise an exception by default when trying to find the first template. But that can be a future refactoring.

@rafaelfranca rafaelfranca merged commit d41d7a4 into rails:master Jul 15, 2016

2 checks passed

codeclimate Code Climate didn't find any new or fixed issues.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@rafaelfranca

This comment has been minimized.

Member

rafaelfranca commented Jul 15, 2016

Backported in 0ef05cd

rafaelfranca added a commit that referenced this pull request Jul 15, 2016

Merge pull request #25826 from javan/actionview/trim-digestor-templat…
…e-lookups

Eliminate extra template lookup in ActionView::Digestor
@javan

This comment has been minimized.

Member

javan commented Jul 15, 2016

❤️

We may want to change the API of PathSet to have a find and find!

That was my instinct too and I agree it should be a separate change.

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