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

Small benchmark refactoring #7211

Merged
merged 1 commit into from Sep 2, 2018

Conversation

Projects
None yet
5 participants
@vbrazo
Contributor

vbrazo commented Sep 2, 2018

Refactoring of some duplicated lines in the benchmark/schwartzian_transform.rb

@oe

oe approved these changes Sep 2, 2018

@ashmaroli

This comment has been minimized.

Member

ashmaroli commented Sep 2, 2018

@vbrazo Please explain the exact code smell that is being fixed here..

@vbrazo

This comment has been minimized.

Contributor

vbrazo commented Sep 2, 2018

@ashmaroli it's actually just a refactoring of some duplicated lines in the benchmark file

@vbrazo vbrazo changed the title from Fix benchmark code smell to Fix benchmark refactoring Sep 2, 2018

@vbrazo vbrazo changed the title from Fix benchmark refactoring to Small benchmark refactoring Sep 2, 2018

@ashmaroli

This comment has been minimized.

Member

ashmaroli commented Sep 2, 2018

@vbrazo Oh, is that all.. I would've refactored it in the following manner, but your implementation is better.. 👍

SAMPLE = {
  "sparse"     => "redirect_from",
  "non-sparse" => "title"
}

SAMPLE.each do |property, meta_key|
  Benchmark.ips do |x|
    x.config(time: 10, warmup: 5)
    x.report("sort_by_property_directly with #{property} property") do
      sort_by_property_directly(site_docs, meta_key)
    end
    x.report("schwartzian_transform with #{property} property") do
      schwartzian_transform(site_docs, meta_key)
    end
    x.compare!
  end
end
@DirtyF

This comment has been minimized.

Member

DirtyF commented Sep 2, 2018

@jekyllbot: merge +dev

@jekyllbot jekyllbot merged commit 397d289 into jekyll:master Sep 2, 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

@vbrazo vbrazo deleted the vbrazo:chores/fix-code-smell branch Sep 6, 2018

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