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 newline after start of yaml header #2211

Merged
merged 5 commits into from Apr 14, 2014

Conversation

Projects
None yet
6 participants
@benbalter
Contributor

benbalter commented Apr 5, 2014

Fixes #2210.

@@ -374,7 +374,7 @@ def has_relative_page?
end
def has_yaml_header?(file)
"---" == File.open(file) { |fd| fd.read(3) }
"---\n" == File.open(file) { |fd| fd.read(4) }

This comment has been minimized.

@parkr

parkr Apr 6, 2014

Member

What if it's a Windows machine? Will this then have to be 5 bytes == "---\r\n"?

This comment has been minimized.

@mscharley

mscharley Apr 6, 2014

Contributor

Can we just use a regex here? /^---\r?\n/

This comment has been minimized.

@parkr

parkr Apr 6, 2014

Member

Should work. @benbalter Can you please update and add a test for this where it won't read in e.g. the PGP key doc?

This comment has been minimized.

@benbalter

benbalter Apr 6, 2014

Contributor

@mscharley I assumed "are the first four characters X" would be faster than regex, although your regex example raises a good question of line endings. @parkr?

This comment has been minimized.

@parkr

parkr Apr 7, 2014

Member

We might have to use regexp for this, but let's benchmark this.

This comment has been minimized.

@benbalter

benbalter Apr 7, 2014

Contributor

let's benchmark this.

That's all you. 😄

This comment has been minimized.

@jpiasetz

jpiasetz Apr 10, 2014

Contributor

What about encoding it with :universal_newline?

This comment has been minimized.

@parkr

parkr Apr 10, 2014

Member
Rehearsal ------------------------------------------------------
equals               0.020000   0.010000   0.030000 (  0.014973)
regexp               0.010000   0.000000   0.010000 (  0.010342)
precompiled regexp   0.010000   0.000000   0.010000 (  0.010796)
--------------------------------------------- total: 0.050000sec

                         user     system      total        real
equals               0.020000   0.000000   0.020000 (  0.012623)
regexp               0.010000   0.000000   0.010000 (  0.006788)
precompiled regexp   0.000000   0.000000   0.000000 (  0.006676)

Looks to me like we could use Regexp no problem...

Code:

require 'benchmark'

content = "---\r\n"
regexp  = Regexp.compile("---\r?\n")

Benchmark.bmbm do |x|
  x.report("equals") { 
    10000.times { content.encode('utf-8', universal_newlines: true) == "---\n" }
  }
  x.report("regexp") { 
    10000.times { !!/---\r?\n/.match(content) }
  }
  x.report("precompiled regexp") { 
    10000.times { regexp.match content }
  }
end
@benbalter

This comment has been minimized.

Contributor

benbalter commented Apr 6, 2014

Tests added. Surprisingly, looks like that method wasn't tested at all.

@parkr parkr added the Fix label Apr 7, 2014

@benbalter

This comment has been minimized.

Contributor

benbalter commented Apr 11, 2014

Thanks @parkr. Regex'd!

@@ -374,7 +374,7 @@ def has_relative_page?
end
def has_yaml_header?(file)
"---" == File.open(file) { |fd| fd.read(3) }
!!(File.open(file).read =~ /^---\r?\n/)

This comment has been minimized.

@parkr

parkr Apr 11, 2014

Member

Do you think we should do \A instead of ^?

This comment has been minimized.

@benbalter

benbalter Apr 11, 2014

Contributor

Great call. Done.

@parkr

This comment has been minimized.

Member

parkr commented Apr 14, 2014

This LGTM.

/cc @mattr-

assert_equal true, @site.send(:has_yaml_header?, abs_path)
end
should "not read PGP keys as pages" do

This comment has been minimized.

@parkr

parkr Apr 14, 2014

Member

What about should "enforce a strict 3-dash limit on the start of the YAML front-matter"? I imagine PGP keys are just one example.

This comment has been minimized.

@benbalter

benbalter Apr 14, 2014

Contributor

Good call. Updated.

@parkr parkr added this to the 2.0 milestone Apr 14, 2014

@parkr

This comment has been minimized.

Member

parkr commented Apr 14, 2014

@benbalter Thanks for dealing with all the stuff I've thrown at you here. I think this is ready to go. And since you're also a collab on the team, I think your and my approval works! Merging.

parkr added a commit that referenced this pull request Apr 14, 2014

@parkr parkr merged commit 4f66db6 into master Apr 14, 2014

1 check passed

continuous-integration/travis-ci The Travis CI build passed
Details

@parkr parkr deleted the pgp-key-header-frontmatter branch Apr 14, 2014

parkr added a commit that referenced this pull request Apr 14, 2014

parkr added a commit that referenced this pull request Apr 15, 2014

@jekyll jekyll locked and limited conversation to collaborators Feb 27, 2017

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