Add option to fail a build with front matter syntax errors #5832
Conversation
|
/cc @jekyll/build |
|
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. |
|
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. |
| rescue => e | ||
| Jekyll.logger.warn "Error reading file #{filename}: #{e.message}" | ||
| raise e if self.site.config["strict_front_matter"] |
There was a problem hiding this comment.
This handling also needs to be added to Jekyll::Document#read, I believe.
There was a problem hiding this comment.
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.
|
Polite bump. I made the requested changes in f904fcc |
|
I think they'll want you to fix the lint issues Travis has found: |
lib/jekyll/document.rb
Outdated
| @@ -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"] | |||
There was a problem hiding this comment.
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
docs/_docs/configuration.md
Outdated
| markdown_ext: "markdown,mkdown,mkdn,mkd,md" | ||
| safe: false | ||
| include: [".htaccess"] | ||
| exclude: ["node_modules", "vendor/bundle/", "vendor/cache/", "vendor/gems/", "vendor/ruby/"] |
There was a problem hiding this comment.
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
|
There seem to be a conflict – can you please take a look? |
|
@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. |
|
@jmhooper You can fix some of the linting with 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. |
|
Thanks for all the great feedback! I've put aside some time this weekend for this one |
|
@DirtyF have you seen this, then you can force Travis to be green before merging, or at least clarify things for contributors: |
|
I pushed a commit to fix some of the linter issues. A few things to note:
The tests and the linter are both green at home for me on ruby 2.3. |
|
Looks like Travis blew up while trying to install faraday? Not sure that's something I touched. |
@jmhooper the |
003a865 to
5eff908
Compare
.rubocop.yml
Outdated
| @@ -41,6 +41,8 @@ Metrics/MethodLength: | |||
| CountComments: false | |||
| Max: 20 | |||
| Severity: error | |||
| Exclude: | |||
| - lib/jekyll/command.rb | |||
There was a problem hiding this comment.
Tsk tsk, that's cheating! Haha. What was the error you got here?
There was a problem hiding this comment.
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"
endWe 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.
There was a problem hiding this comment.
@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
There was a problem hiding this comment.
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
There was a problem hiding this comment.
You can split the def
add_build_optionsinto 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
lib/jekyll/document.rb
Outdated
| @@ -260,10 +260,7 @@ def read(opts = {}) | |||
| read_content(opts) | |||
| read_post_data | |||
| rescue SyntaxError => e | |||
There was a problem hiding this comment.
Doesn't this have to be rescue => e so it captures all errors?
lib/jekyll/document.rb
Outdated
| else | ||
| Jekyll.logger.error "Error:", "could not read file #{path}: #{error.message}" | ||
| end | ||
| raise error if site.config["strict_front_matter"] |
There was a problem hiding this comment.
This also misses the Jekyll::Errors::FatalException exception.
5eff908 to
3081e78
Compare
|
@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 |
|
For the test, I've got something like this in 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
endAlso 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. |
| "keep_files" => [".git", ".svn"], | ||
| "encoding" => "utf-8", | ||
| "markdown_ext" => "markdown,mkdown,mkdn,mkd,md", | ||
| "strict_front_matter" => false, |
There was a problem hiding this comment.
This is the only real change in this file; everything else is alignment 👍
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.
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`.
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
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.
6d638a9 to
83e2e2e
Compare
|
I went ahead and pushed the test for the config on the site that I was talking about above ☝️ |
|
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 |
|
@jekyllbot: merge +minor |
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 issue I run into is that
jekyll buildis 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:
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 (#5257, #5485, #4713). Those have all been closed.
Additionally, there is WIP PR that looks like it may pick this up (#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:
Thanks for reading! Hope you'll consider this change!