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

Projects
None yet
5 participants
@ashmaroli
Member

ashmaroli commented Dec 4, 2017

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

@ashmaroli ashmaroli requested a review from pathawks Dec 4, 2017

@Crunch09

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

parkr approved these changes Jan 14, 2018

@parkr

This comment has been minimized.

Member

parkr commented Jan 14, 2018

@jekyllbot: merge +minor

@jekyllbot jekyllbot merged commit 9a88900 into jekyll:master Jan 14, 2018

3 checks passed

WIP ready for review
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@ashmaroli ashmaroli deleted the ashmaroli:require-once branch Jan 15, 2018

@ashmaroli

This comment has been minimized.

Member

ashmaroli commented Jan 15, 2018

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

@parkr

This comment has been minimized.

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

This comment has been minimized.

Member

pathawks commented Jan 15, 2018

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

This comment has been minimized.

Member

ashmaroli commented Jan 16, 2018

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.. 😁

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