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

Require external library only if necessary #6596

Merged
merged 1 commit into from Jan 14, 2018

Conversation

ashmaroli
Copy link
Member

Even though the Ruby interpreter does not require a library more than once, there's scope for optimization by not initiating Jekyll::External.require_with_graceful_fail unless necessary within current build session

Copy link
Member

@Crunch09 Crunch09 left a comment

Choose a reason for hiding this comment

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

Do you have a benchmark to see how much this will save us? I don't feel that the decreased readability is worth the change

@parkr
Copy link
Member

parkr commented Jan 14, 2018

@jekyllbot: merge +minor

@jekyllbot jekyllbot merged commit 9a88900 into jekyll:master Jan 14, 2018
jekyllbot added a commit that referenced this pull request Jan 14, 2018
@ashmaroli ashmaroli deleted the require-once branch January 15, 2018 05:41
@ashmaroli
Copy link
Member Author

@parkr this should've been marked as a bug-fix change instead of minor-enhancement

@parkr
Copy link
Member

parkr commented Jan 15, 2018

@ashmaroli ruby’s require method is highly cached so there wasn’t really a bug or performance problem. We can reclassify if you want.

@pathawks
Copy link
Member

ruby’s require method is highly cached so there wasn’t really a bug or performance problem.

In that case, what advantage does this change provide?

@ashmaroli
Copy link
Member Author

so there wasn’t really a bug or performance problem.

oi.. so this was an unnecessary change..?

We can reclassify if you want.

Depends on whether the next release is a v3.7.1 or v3.8.0.. 😁

@jekyll jekyll locked and limited conversation to collaborators Jul 12, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants