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

Replace simple regex with equivalent Ruby methods #6736

Merged
merged 4 commits into from Feb 20, 2018

Conversation

Projects
None yet
5 participants
@ashmaroli
Member

ashmaroli commented Feb 1, 2018

Prefer built-in functions over using simple regular expressions to produce same result

@oe

oe approved these changes Feb 1, 2018

@ashmaroli ashmaroli changed the title from Replace simple regex with equivalent Ruby methods to WIP: Replace simple regex with equivalent Ruby methods Feb 1, 2018

@ashmaroli ashmaroli changed the title from WIP: Replace simple regex with equivalent Ruby methods to Replace simple regex with equivalent Ruby methods Feb 1, 2018

@ashmaroli

This comment has been minimized.

Member

ashmaroli commented Feb 12, 2018

text = <<-STR
alpha
beta gamma
delta 
STR
# => "alpha\nbeta gamma\ndelta "

text.split
# => ["alpha", "beta", "gamma", "delta"]

@ashmaroli ashmaroli referenced this pull request Feb 17, 2018

Closed

Release 3.8.0 #6783

12 of 13 tasks complete

@DirtyF DirtyF requested a review from jekyll/performance Feb 18, 2018

@parkr

This comment has been minimized.

Member

parkr commented Feb 20, 2018

Results of the benchmark:

~/jekyll/jekyll#ashmaroli-replace-regex$ ruby --version
ruby 2.5.0p0 (2017-12-25 revision 61468) [x86_64-darwin17]
~/jekyll/jekyll#ashmaroli-replace-regex$ be ruby benchmark/sanitize-url.rb
Warming up --------------------------------------
 sanitize w/ regexes    15.115k i/100ms
 sanitize w/ builtin    23.283k i/100ms
Calculating -------------------------------------
 sanitize w/ regexes    153.544k (± 6.1%) i/s -    770.865k in   5.038638s
 sanitize w/ builtin    244.131k (± 6.3%) i/s -      1.234M in   5.074822s

Comparison:
 sanitize w/ builtin:   244131.2 i/s
 sanitize w/ regexes:   153544.1 i/s - 1.59x  slower
@parkr

parkr approved these changes Feb 20, 2018

Nice and thorough. Great attention to detail! 👍

@@ -417,7 +417,7 @@ def populate_tags
def merge_categories!(other)
if other.key?("categories") && !other["categories"].nil?
if other["categories"].is_a?(String)
other["categories"] = other["categories"].split(%r!\s+!).map(&:strip)
other["categories"] = other["categories"].split

This comment has been minimized.

@parkr

parkr Feb 20, 2018

Member

👍

>> " hello    \n friends".split
=> ["hello", "friends"]
@@ -8,7 +8,7 @@ class Jekyll::ThemeBuilder
attr_reader :name, :path, :code_of_conduct
def initialize(theme_name, opts)
@name = theme_name.to_s.tr(" ", "_").gsub(%r!_+!, "_")
@name = theme_name.to_s.tr(" ", "_").squeeze("_")

This comment has been minimized.

@parkr

parkr Feb 20, 2018

Member

I had no idea this method existed! Great find!

>> "hello____friends___".squeeze("_")
=> "hello_friends_"

This comment has been minimized.

@ashmaroli

ashmaroli Feb 20, 2018

Member

Thanks.. we do use squeeze somewhere else in the codebase IIRC.. 😃

end
# Returns a sanitized String URL, stripping "../../" and multiples of "/",
# as well as the beginning "/" so we can enforce and ensure it.
def sanitize_url(str)
"/" + str.gsub(%r!/{2,}!, "/").gsub(%r!\.+/|\A/+!, "")
"/#{str}".gsub("..", "/").gsub("./", "").squeeze("/")

This comment has been minimized.

@parkr

parkr Feb 20, 2018

Member

Using "/#{str}" subtly changes things, because now .gsub is operating on the string with the prepended /, rather than just the input. CI passes though, so 👍

This comment has been minimized.

@ashmaroli

ashmaroli Feb 20, 2018

Member

that was my intention.. to have the input prepended with a / at all times and then proceed with the sanitization..
The benchmark attached, shows that its better.. 😃

@DirtyF

DirtyF approved these changes Feb 20, 2018

Nice work @ashmaroli 🚀

@oe

This comment has been minimized.

Member

oe commented Feb 20, 2018

@jekyllbot: merge +dev

@jekyllbot jekyllbot merged commit a5c25ad into jekyll:master Feb 20, 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:replace-regex branch Feb 20, 2018

@ashmaroli ashmaroli added this to the v3.8.0 milestone Feb 20, 2018

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