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

Remove override to Jekyll::Document#respond_to? #7695

Merged
merged 1 commit into from Jun 5, 2019

Conversation

Projects
None yet
4 participants
@ashmaroli
Copy link
Member

commented Jun 5, 2019

Summary

Overriding of #respond_to? is unnecessary when #method_missing and #respond_to_missing? has been overridden.

Background

Based on the Ruby docs

Returns true if obj responds to the given method.
If the method is not defined, respond_to_missing? method is called and the result is returned.

Document#repond_to_missing? has the same logic as Document#respond_to?;

def respond_to_missing?(method, *)
data.key?(method.to_s) || super
end

def respond_to?(method, include_private = false)
data.key?(method.to_s) || super
end

Benefits

:respond_to? is a popular method that is almost always called with a Symbol argument.
But because we coerce the argument to string in ln#398, a new string is invariably allocated on every call. Removing this override defers those string allocations for methods that are not defined for Jekyll::Document

Note: I did not include tests because calling a Document with a key included in the doc's data hash is a deprecated feature.

@ashmaroli ashmaroli requested a review from jekyll/core Jun 5, 2019

@ashmaroli

This comment has been minimized.

Copy link
Member Author

commented Jun 5, 2019

Profiler summary

--- master branch https://travis-ci.org/jekyll/jekyll/jobs/541575706
+++ PR branch     https://travis-ci.org/jekyll/jekyll/jobs/541632941

- Total allocated: 549.57 MB (4783239 objects)
+ Total allocated: 548.75 MB (4762586 objects)
  Total retained:  18.28 MB (95954 objects)
@parkr

parkr approved these changes Jun 5, 2019

@mattr-

This comment has been minimized.

Copy link
Member

commented Jun 5, 2019

@jekyllbot: merge +dev

@jekyllbot jekyllbot merged commit e057459 into jekyll:master Jun 5, 2019

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
netlify/jekyllrb/deploy-preview Deploy preview ready!
Details

jekyllbot added a commit that referenced this pull request Jun 5, 2019

@ashmaroli ashmaroli deleted the ashmaroli:document-respond_to branch Jun 5, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.