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

Add option to fail a build with front matter syntax errors #5832

Merged
merged 5 commits into from May 10, 2017

Conversation

Projects
None yet
8 participants
@jmhooper
Contributor

jmhooper commented Jan 30, 2017

This is a feature request by way of a PR for a problem I've had with Jekyll in past.

I have a number of sites that are deployed from a CI. The setup is functionally equivalent to the results of following Jekyll's own documentation on using a CI:

The simplest test script simply runs jekyll build and ensures that Jekyll doesn’t fail to build the site. It doesn’t check the resulting site, but it does ensure things are built properly.

The issue I run into is that jekyll build is not sufficient to verify that a site is building as expected. If there is a syntax error in a page's front matter, Jekyll will print a warning, ignore the page, and continue.

This repeatedly causes problems for me when people add a page and add something like this in the front matter:

title: The question: Will this build
description: 'Tough question. It's answer may surprise you'

The build succeeds on CI, and then the site is deployed with that page missing, which is not expected behavior.

This situation has been mentioned a few times in issues (jekyll/jekyll#5257, jekyll/jekyll#5485, jekyll/jekyll#4713). Those have all been closed.

Additionally, there is WIP PR that looks like it may pick this up (jekyll/jekyll#4674), but it also looks like there hasn't been any movement there since March 2016.

Here's the message for the commit in this PR which describes what it does:

This commit adds a config named strict_front_matter. This configuration causes a build to fail if there is a syntax error in a page's front matter.

Prior to this commit, if a page had an error in its front matter the build would print a warning, ignore the file, and continue. This caused problems where pages would have a syntax error and the build would not fail, and instead build an incomplete site.

This commit adds the option which tells a build to fail when there is a syntax error reading / parsing a page's front matter. This is done by setting the strict_front_matter config or using the --strict_front_matter CLI option.

This commit also adds a test case for this behavior and updates the documentation.

Thanks for reading! Hope you'll consider this change!

@DirtyF

This comment has been minimized.

Show comment
Hide comment
@DirtyF
Member

DirtyF commented Jan 30, 2017

@peternewman

This comment has been minimized.

Show comment
Hide comment
@peternewman

peternewman Feb 7, 2017

Thanks for this, I'll certainly be grateful of it.

It looks like you've failed some of their lint tests, at least one of which is unrelated to you and has been fixed upstream.

peternewman commented Feb 7, 2017

Thanks for this, I'll certainly be grateful of it.

It looks like you've failed some of their lint tests, at least one of which is unrelated to you and has been fixed upstream.

@wjdp

This comment has been minimized.

Show comment
Hide comment
@wjdp

wjdp Feb 7, 2017

Member

Just to add a thought to this. I debugged some work an editor had done today where they added a front matter variable and then observed their change didn't take effect. What'd happened was a second definition of that variable was present father down the front matter, which hadn't been noticed. This masked any change the editor made.

Long story short: what would be fantastic is for strict checking to also throw an error if a variable is defined twice.

Member

wjdp commented Feb 7, 2017

Just to add a thought to this. I debugged some work an editor had done today where they added a front matter variable and then observed their change didn't take effect. What'd happened was a second definition of that variable was present father down the front matter, which hadn't been noticed. This masked any change the editor made.

Long story short: what would be fantastic is for strict checking to also throw an error if a variable is defined twice.

@peternewman peternewman referenced this pull request Feb 7, 2017

Open

Travis TODO #184

3 of 8 tasks complete
@parkr

I dig it!

rescue => e
Jekyll.logger.warn "Error reading file #{filename}: #{e.message}"
raise e if self.site.config["strict_front_matter"]

This comment has been minimized.

@parkr

parkr Feb 11, 2017

Member

This handling also needs to be added to Jekyll::Document#read, I believe.

@parkr

parkr Feb 11, 2017

Member

This handling also needs to be added to Jekyll::Document#read, I believe.

This comment has been minimized.

@jmhooper

jmhooper Feb 11, 2017

Contributor

Cool. Saw that, but wasn't sure where Jekyll::Document#read was used so I left it off. Commit to add it is up now.

@jmhooper

jmhooper Feb 11, 2017

Contributor

Cool. Saw that, but wasn't sure where Jekyll::Document#read was used so I left it off. Commit to add it is up now.

@jmhooper

This comment has been minimized.

Show comment
Hide comment
@jmhooper

jmhooper Mar 4, 2017

Contributor

Polite bump.

I made the requested changes in f904fcc

Contributor

jmhooper commented Mar 4, 2017

Polite bump.

I made the requested changes in f904fcc

@peternewman

This comment has been minimized.

Show comment
Hide comment
@peternewman

peternewman Mar 4, 2017

I think they'll want you to fix the lint issues Travis has found:
https://travis-ci.org/jekyll/jekyll/jobs/200753547

peternewman commented Mar 4, 2017

I think they'll want you to fix the lint issues Travis has found:
https://travis-ci.org/jekyll/jekyll/jobs/200753547

Show outdated Hide outdated lib/jekyll/document.rb
@@ -258,9 +258,11 @@ def read(opts = {})
read_post_data
rescue SyntaxError => e
Jekyll.logger.error "Error:", "YAML Exception reading #{path}: #{e.message}"
raise e if self.site.config["strict_front_matter"]

This comment has been minimized.

@ashmaroli

ashmaroli Mar 5, 2017

Member

self is not required here and in the line further below.

- raise e if self.site.config["strict_front_matter"]
+ raise e if site.config["strict_front_matter"]

I also think multiple raises can be avoided by using the or operator:

 raise e if (e.is_a? Jekyll::Errors::FatalException || site.config["strict_front_matter"])
 Jekyll.logger.error "Error:", "could not read file #{path}: #{e.message}"

/cc @pathawks

@ashmaroli

ashmaroli Mar 5, 2017

Member

self is not required here and in the line further below.

- raise e if self.site.config["strict_front_matter"]
+ raise e if site.config["strict_front_matter"]

I also think multiple raises can be avoided by using the or operator:

 raise e if (e.is_a? Jekyll::Errors::FatalException || site.config["strict_front_matter"])
 Jekyll.logger.error "Error:", "could not read file #{path}: #{e.message}"

/cc @pathawks

Show outdated Hide outdated docs/_docs/configuration.md
markdown_ext: "markdown,mkdown,mkdn,mkd,md"
safe: false
include: [".htaccess"]
exclude: ["node_modules", "vendor/bundle/", "vendor/cache/", "vendor/gems/", "vendor/ruby/"]

This comment has been minimized.

@ashmaroli

ashmaroli Mar 5, 2017

Member

Note: To be fixed on master or via another pull

exclude: array is now ["Gemfile", "Gemfile.lock", "node_modules", "vendor/bundle/", "vendor/cache/", "vendor/gems/", "vendor/ruby/"]

/cc @DirtyF

@ashmaroli

ashmaroli Mar 5, 2017

Member

Note: To be fixed on master or via another pull

exclude: array is now ["Gemfile", "Gemfile.lock", "node_modules", "vendor/bundle/", "vendor/cache/", "vendor/gems/", "vendor/ruby/"]

/cc @DirtyF

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Mar 31, 2017

Member

There seem to be a conflict – can you please take a look?

Member

parkr commented Mar 31, 2017

There seem to be a conflict – can you please take a look?

@DirtyF

This comment has been minimized.

Show comment
Hide comment
@DirtyF

DirtyF Mar 31, 2017

Member

@parkr fixed (was because of #5947)

Member

DirtyF commented Mar 31, 2017

@parkr fixed (was because of #5947)

@peternewman

This comment has been minimized.

Show comment
Hide comment
@peternewman

peternewman Mar 31, 2017

@DirtyF there still seem to be some lint issues I think they'll want to you fix before merging. And a test failure which I suspect isn't your fault.

peternewman commented Mar 31, 2017

@DirtyF there still seem to be some lint issues I think they'll want to you fix before merging. And a test failure which I suspect isn't your fault.

@DirtyF

This comment has been minimized.

Show comment
Hide comment
@DirtyF

DirtyF Mar 31, 2017

Member

@jmhooper You can fix some of the linting with script/fmt -a

CI failing test seems unrelated: https://travis-ci.org/jekyll/jekyll/jobs/217037611 and only fails with Ruby 2.4 that we don't seem to fully support yet so it shouldn't be a blocker.

Member

DirtyF commented Mar 31, 2017

@jmhooper You can fix some of the linting with script/fmt -a

CI failing test seems unrelated: https://travis-ci.org/jekyll/jekyll/jobs/217037611 and only fails with Ruby 2.4 that we don't seem to fully support yet so it shouldn't be a blocker.

@jmhooper

This comment has been minimized.

Show comment
Hide comment
@jmhooper

jmhooper Mar 31, 2017

Contributor

Thanks for all the great feedback! I've put aside some time this weekend for this one

Contributor

jmhooper commented Mar 31, 2017

Thanks for all the great feedback! I've put aside some time this weekend for this one

@peternewman

This comment has been minimized.

Show comment
Hide comment
@peternewman

peternewman Mar 31, 2017

@DirtyF have you seen this, then you can force Travis to be green before merging, or at least clarify things for contributors:
https://docs.travis-ci.com/user/customizing-the-build#Rows-that-are-Allowed-to-Fail

peternewman commented Mar 31, 2017

@DirtyF have you seen this, then you can force Travis to be green before merging, or at least clarify things for contributors:
https://docs.travis-ci.com/user/customizing-the-build#Rows-that-are-Allowed-to-Fail

@jmhooper

This comment has been minimized.

Show comment
Hide comment
@jmhooper

jmhooper Apr 1, 2017

Contributor

I pushed a commit to fix some of the linter issues. A few things to note:

1. I added changes I proposed in #5828 to this, along with changes asked for there (rescues from Psych::SyntaxError as well as SyntaxError)
2. I excluded command.rb from the method length cop in .rubocop.yml. The add_build_options method was over 20 lines and I couldn't see a way to get it below that. Lmk if that's not acceptable and I can break it up, but I am not sure what the best way to do that is.
3. I reduced the complexity of read in document.rb by adding a private method to handle errors. I decided to go that route to keep the descriptive error message about YAML parsing issues.

The tests and the linter are both green at home for me on ruby 2.3.

Contributor

jmhooper commented Apr 1, 2017

I pushed a commit to fix some of the linter issues. A few things to note:

1. I added changes I proposed in #5828 to this, along with changes asked for there (rescues from Psych::SyntaxError as well as SyntaxError)
2. I excluded command.rb from the method length cop in .rubocop.yml. The add_build_options method was over 20 lines and I couldn't see a way to get it below that. Lmk if that's not acceptable and I can break it up, but I am not sure what the best way to do that is.
3. I reduced the complexity of read in document.rb by adding a private method to handle errors. I decided to go that route to keep the descriptive error message about YAML parsing issues.

The tests and the linter are both green at home for me on ruby 2.3.

@jmhooper

This comment has been minimized.

Show comment
Hide comment
@jmhooper

jmhooper Apr 1, 2017

Contributor

Looks like Travis blew up while trying to install faraday? Not sure that's something I touched.

Contributor

jmhooper commented Apr 1, 2017

Looks like Travis blew up while trying to install faraday? Not sure that's something I touched.

@ashmaroli

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Apr 2, 2017

Member

Looks like Travis blew up while trying to install faraday? Not sure that's something I touched.

@jmhooper the checksum issue with gem faraday has been resolved upstream. The tests should run as usual when one of the maintainers here will restart the ci builds for you.

Member

ashmaroli commented Apr 2, 2017

Looks like Travis blew up while trying to install faraday? Not sure that's something I touched.

@jmhooper the checksum issue with gem faraday has been resolved upstream. The tests should run as usual when one of the maintainers here will restart the ci builds for you.

@jmhooper

This comment has been minimized.

Show comment
Hide comment
@jmhooper

jmhooper Apr 3, 2017

Contributor

I amended the commit to remove the first point above per feedback from @parkr on #5832 🙂

Contributor

jmhooper commented Apr 3, 2017

I amended the commit to remove the first point above per feedback from @parkr on #5832 🙂

Show outdated Hide outdated .rubocop.yml
@@ -41,6 +41,8 @@ Metrics/MethodLength:
CountComments: false
Max: 20
Severity: error
Exclude:
- lib/jekyll/command.rb

This comment has been minimized.

@parkr

parkr Apr 5, 2017

Member

Tsk tsk, that's cheating! Haha. What was the error you got here?

@parkr

parkr Apr 5, 2017

Member

Tsk tsk, that's cheating! Haha. What was the error you got here?

This comment has been minimized.

@jmhooper

jmhooper Apr 6, 2017

Contributor

This one was tricky for me. The offending method is here:

      def add_build_options(c)
        c.option "config", "--config CONFIG_FILE[,CONFIG_FILE2,...]",
          Array, "Custom configuration file"
        c.option "destination", "-d", "--destination DESTINATION",
          "The current folder will be generated into DESTINATION"
        c.option "source", "-s", "--source SOURCE", "Custom source directory"
        c.option "future", "--future", "Publishes posts with a future date"
        c.option "limit_posts", "--limit_posts MAX_POSTS", Integer,
          "Limits the number of posts to parse and publish"
        c.option "watch", "-w", "--[no-]watch", "Watch for changes and rebuild"
        c.option "baseurl", "-b", "--baseurl URL",
          "Serve the website from the given base URL"
        c.option "force_polling", "--force_polling", "Force watch to use polling"
        c.option "lsi", "--lsi", "Use LSI for improved related posts"
        c.option "show_drafts", "-D", "--drafts", "Render posts in the _drafts folder"
        c.option "unpublished", "--unpublished",
          "Render posts that were marked as unpublished"
        c.option "quiet", "-q", "--quiet", "Silence output."
        c.option "verbose", "-V", "--verbose", "Print verbose output."
        c.option "incremental", "-I", "--incremental", "Enable incremental rebuild."
        c.option "strict_front_matter", "--strict_front_matter",
          "Fail if errors are present in front matter"
      end

We get dinged b/c it's over 20 lines. It's right at 21 lines. Coalescing any of the options that cover multiple lines brings the line length over 90 characters. Breaking it into add_build_options_a_through_m and add_build_options_n_through_z is the next best thing I could think of, which would be ridiculous of course.

I'm welcoming thoughts on this b/c I agree that changing the cop is cheating.

@jmhooper

jmhooper Apr 6, 2017

Contributor

This one was tricky for me. The offending method is here:

      def add_build_options(c)
        c.option "config", "--config CONFIG_FILE[,CONFIG_FILE2,...]",
          Array, "Custom configuration file"
        c.option "destination", "-d", "--destination DESTINATION",
          "The current folder will be generated into DESTINATION"
        c.option "source", "-s", "--source SOURCE", "Custom source directory"
        c.option "future", "--future", "Publishes posts with a future date"
        c.option "limit_posts", "--limit_posts MAX_POSTS", Integer,
          "Limits the number of posts to parse and publish"
        c.option "watch", "-w", "--[no-]watch", "Watch for changes and rebuild"
        c.option "baseurl", "-b", "--baseurl URL",
          "Serve the website from the given base URL"
        c.option "force_polling", "--force_polling", "Force watch to use polling"
        c.option "lsi", "--lsi", "Use LSI for improved related posts"
        c.option "show_drafts", "-D", "--drafts", "Render posts in the _drafts folder"
        c.option "unpublished", "--unpublished",
          "Render posts that were marked as unpublished"
        c.option "quiet", "-q", "--quiet", "Silence output."
        c.option "verbose", "-V", "--verbose", "Print verbose output."
        c.option "incremental", "-I", "--incremental", "Enable incremental rebuild."
        c.option "strict_front_matter", "--strict_front_matter",
          "Fail if errors are present in front matter"
      end

We get dinged b/c it's over 20 lines. It's right at 21 lines. Coalescing any of the options that cover multiple lines brings the line length over 90 characters. Breaking it into add_build_options_a_through_m and add_build_options_n_through_z is the next best thing I could think of, which would be ridiculous of course.

I'm welcoming thoughts on this b/c I agree that changing the cop is cheating.

This comment has been minimized.

@ashmaroli

ashmaroli Apr 6, 2017

Member

@jmhooper You can split the def add_build_options into smaller methods (IMO):

    def add_build_options(c)
      c.option "config", "--config CONFIG_FILE[,CONFIG_FILE2,...]",
        Array, "Custom configuration file"
      c.option "destination", "-d", "--destination DESTINATION",
        "The current folder will be generated into DESTINATION"
      c.option "source", "-s", "--source SOURCE", "Custom source directory"
      c.option "future", "--future", "Publishes posts with a future date"
      c.option "limit_posts", "--limit_posts MAX_POSTS", Integer,
        "Limits the number of posts to parse and publish"
      c.option "watch", "-w", "--[no-]watch", "Watch for changes and rebuild"
      c.option "baseurl", "-b", "--baseurl URL",
        "Serve the website from the given base URL"
      c.option "force_polling", "--force_polling", "Force watch to use polling"
      c.option "lsi", "--lsi", "Use LSI for improved related posts"
      c.option "show_drafts", "-D", "--drafts", "Render posts in the _drafts folder"
      c.option "unpublished", "--unpublished",
        "Render posts that were marked as unpublished"
      c.option "quiet", "-q", "--quiet", "Silence output."
      c.option "verbose", "-V", "--verbose", "Print verbose output."

      additional_build_options(c)
    end

    private

    def additional_build_options(c)
      c.option "incremental", "-I", "--incremental", "Enable incremental rebuild."
      c.option "strict_front_matter", "--strict_front_matter",
        "Fail if errors are present in front matter"
    end
      
@ashmaroli

ashmaroli Apr 6, 2017

Member

@jmhooper You can split the def add_build_options into smaller methods (IMO):

    def add_build_options(c)
      c.option "config", "--config CONFIG_FILE[,CONFIG_FILE2,...]",
        Array, "Custom configuration file"
      c.option "destination", "-d", "--destination DESTINATION",
        "The current folder will be generated into DESTINATION"
      c.option "source", "-s", "--source SOURCE", "Custom source directory"
      c.option "future", "--future", "Publishes posts with a future date"
      c.option "limit_posts", "--limit_posts MAX_POSTS", Integer,
        "Limits the number of posts to parse and publish"
      c.option "watch", "-w", "--[no-]watch", "Watch for changes and rebuild"
      c.option "baseurl", "-b", "--baseurl URL",
        "Serve the website from the given base URL"
      c.option "force_polling", "--force_polling", "Force watch to use polling"
      c.option "lsi", "--lsi", "Use LSI for improved related posts"
      c.option "show_drafts", "-D", "--drafts", "Render posts in the _drafts folder"
      c.option "unpublished", "--unpublished",
        "Render posts that were marked as unpublished"
      c.option "quiet", "-q", "--quiet", "Silence output."
      c.option "verbose", "-V", "--verbose", "Print verbose output."

      additional_build_options(c)
    end

    private

    def additional_build_options(c)
      c.option "incremental", "-I", "--incremental", "Enable incremental rebuild."
      c.option "strict_front_matter", "--strict_front_matter",
        "Fail if errors are present in front matter"
    end
      

This comment has been minimized.

@ashmaroli

ashmaroli Apr 6, 2017

Member

If that's not acceptable, there's another hack. c.option "source" is technically a duplicate as the jekyll executable already adds that option to all Jekyll Commands. So you can remove that one line from here.. again, just my 2c

@ashmaroli

ashmaroli Apr 6, 2017

Member

If that's not acceptable, there's another hack. c.option "source" is technically a duplicate as the jekyll executable already adds that option to all Jekyll Commands. So you can remove that one line from here.. again, just my 2c

This comment has been minimized.

@pathawks

pathawks Apr 6, 2017

Member

You can split the def add_build_options into smaller methods

Nope. Not doing that.

Just exclude this one method from the MethodLength cop.

# rubocop:disable Metrics/MethodLength
def add_build_options(c)
    ....
end
# rubocop:enable Metrics/MethodLength
@pathawks

pathawks Apr 6, 2017

Member

You can split the def add_build_options into smaller methods

Nope. Not doing that.

Just exclude this one method from the MethodLength cop.

# rubocop:disable Metrics/MethodLength
def add_build_options(c)
    ....
end
# rubocop:enable Metrics/MethodLength

This comment has been minimized.

@pathawks

pathawks Apr 6, 2017

Member

I got you @jmhooper 🍻

@pathawks

pathawks Apr 6, 2017

Member

I got you @jmhooper 🍻

This comment has been minimized.

@jmhooper

jmhooper Apr 6, 2017

Contributor

Cheers!

@jmhooper

jmhooper Apr 6, 2017

Contributor

Cheers!

Show outdated Hide outdated lib/jekyll/document.rb
@@ -260,10 +260,7 @@ def read(opts = {})
read_content(opts)
read_post_data
rescue SyntaxError => e

This comment has been minimized.

@parkr

parkr Apr 5, 2017

Member

Doesn't this have to be rescue => e so it captures all errors?

@parkr

parkr Apr 5, 2017

Member

Doesn't this have to be rescue => e so it captures all errors?

Show outdated Hide outdated lib/jekyll/document.rb
else
Jekyll.logger.error "Error:", "could not read file #{path}: #{error.message}"
end
raise error if site.config["strict_front_matter"]

This comment has been minimized.

@parkr

parkr Apr 5, 2017

Member

This also misses the Jekyll::Errors::FatalException exception.

@parkr

parkr Apr 5, 2017

Member

This also misses the Jekyll::Errors::FatalException exception.

@jmhooper

This comment has been minimized.

Show comment
Hide comment
@jmhooper

jmhooper Apr 6, 2017

Contributor

@parkr: I amended the commit to address your last 2 comments. I also commented on the first ones looking for some guidance.

The fact that the last 2 comments weren't picked up by the tests made me realize that when I did this originally I only added tests to cover Convertible. I'd like to add tests to cover the code in Document as well, but test_document.rb doesn't look like a good fit for that oddly enough? I'm currently working on adding them to test_site.rb which seems like a better fit. Interested in any thoughts folks here have on where the test should go.

Contributor

jmhooper commented Apr 6, 2017

@parkr: I amended the commit to address your last 2 comments. I also commented on the first ones looking for some guidance.

The fact that the last 2 comments weren't picked up by the tests made me realize that when I did this originally I only added tests to cover Convertible. I'd like to add tests to cover the code in Document as well, but test_document.rb doesn't look like a good fit for that oddly enough? I'm currently working on adding them to test_site.rb which seems like a better fit. Interested in any thoughts folks here have on where the test should go.

@jmhooper

This comment has been minimized.

Show comment
Hide comment
@jmhooper

jmhooper Apr 6, 2017

Contributor

For the test, I've got something like this in test_site.rb:

class TestSite < JekyllUnitTest

  # ...

  context "creating sites" do

    # ...

    context "error handling" do

      # ...

      should "raise for bad frontmatter if strict_front_matter is set" do
        site = Site.new(site_configuration(
          "collections" => ["broken"],
          "strict_front_matter" => true,
        ))
        assert_raises do
          site.process
        end
      end

      should "not raise for bad frontmatter if strict_front_matter is not set" do
        site = Site.new(site_configuration(
          "collections" => ["broken"],
          "strict_front_matter" => false,
        ))
        site.process
      end
    end
  end
end

Also note that I've added a folder to the test site named "_broken". It has a single markdown file in it with malformed frontmatter.

I haven't committed yet b/c I'm interested to see how folks feel about where I've got this.

Contributor

jmhooper commented Apr 6, 2017

For the test, I've got something like this in test_site.rb:

class TestSite < JekyllUnitTest

  # ...

  context "creating sites" do

    # ...

    context "error handling" do

      # ...

      should "raise for bad frontmatter if strict_front_matter is set" do
        site = Site.new(site_configuration(
          "collections" => ["broken"],
          "strict_front_matter" => true,
        ))
        assert_raises do
          site.process
        end
      end

      should "not raise for bad frontmatter if strict_front_matter is not set" do
        site = Site.new(site_configuration(
          "collections" => ["broken"],
          "strict_front_matter" => false,
        ))
        site.process
      end
    end
  end
end

Also note that I've added a folder to the test site named "_broken". It has a single markdown file in it with malformed frontmatter.

I haven't committed yet b/c I'm interested to see how folks feel about where I've got this.

@pathawks

Love this! I think a lot of sites are going to be adding this option to their CI tests.

Excellent work @jmhooper! 🎈🤠

"keep_files" => [".git", ".svn"],
"encoding" => "utf-8",
"markdown_ext" => "markdown,mkdown,mkdn,mkd,md",
"strict_front_matter" => false,

This comment has been minimized.

@pathawks

pathawks Apr 6, 2017

Member

This is the only real change in this file; everything else is alignment 👍

@pathawks

pathawks Apr 6, 2017

Member

This is the only real change in this file; everything else is alignment 👍

jmhooper and others added some commits Jan 30, 2017

Add option to fail a build with front matter syntax errors
This commit adds a config named `strict_front_matter`. This configuration causes a build to fail if there is a syntax error in a page's front matter.

Prior to this commit, if a page had an error in its front matter the build would print a warning, ignore the file, and continue. This caused problems where pages would have a syntax error and the build would not fail, and instead build an incomplete site.

This commit adds the option which tells a build to fail when there is a syntax error reading / parsing a page's front matter. This is done by setting the `strict_front_matter` config or using the `--strict_front_matter` CLI option.

This commit also adds a test case for this behavior and updates the documentation.
Add strict front matter to Jekyll::Document
This code adds the logic that enforces strict front matter to the Jekyll::Document class's `read` instance method. This will raise an error if there is a syntax issue in the YAML and the `strict_front_matter` config is set to `true`.
Fix linter issues for strict front matter code
This commit fixes some issues the linter found in the code that added the `--sctrict-front-matter` feature:

- Exlude `command.rb` from the method length requirement since it uses the mercenary DSL to add command options
- Align hash entries in `configuration.rb`
- Reduce complexity of the `read` method in `document.rb` by moving error handling into a private function
- Fix line length in convertible test by reducing verbosity of the example's description
Add tests for strict front matter behavior
This commit adds a test to the site that tests that it does or does not raise for YAML front matter errors depending on the strict front matter setting in the config.
@jmhooper

This comment has been minimized.

Show comment
Hide comment
@jmhooper

jmhooper Apr 30, 2017

Contributor

I went ahead and pushed the test for the config on the site that I was talking about above ☝️

Contributor

jmhooper commented Apr 30, 2017

I went ahead and pushed the test for the config on the site that I was talking about above ☝️

@jmhooper

This comment has been minimized.

Show comment
Hide comment
@jmhooper

jmhooper Apr 30, 2017

Contributor

If anyone stumbles on this and needs the functionality, or wants to add it to a site on an older version of Jekyll, I've created a plugin that mimics the behavior: https://github.com/jmhooper/jekyll_strict_front_matter

Contributor

jmhooper commented Apr 30, 2017

If anyone stumbles on this and needs the functionality, or wants to add it to a site on an older version of Jekyll, I've created a plugin that mimics the behavior: https://github.com/jmhooper/jekyll_strict_front_matter

@parkr

parkr approved these changes May 10, 2017

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr May 10, 2017

Member

@jekyllbot: merge +minor

Member

parkr commented May 10, 2017

@jekyllbot: merge +minor

@jekyllbot jekyllbot merged commit 308ba55 into jekyll:master May 10, 2017

2 checks passed

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

jekyllbot added a commit that referenced this pull request May 10, 2017

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