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

Assert raising Psych::SyntaxError when `"strict_front_matter"=>true` #6520

Merged
merged 3 commits into from Nov 6, 2017

Conversation

Projects
None yet
6 participants
@ashmaroli
Member

ashmaroli commented Nov 5, 2017

Resolves #6515 (as well..)

@@ -266,7 +266,7 @@ def read(opts = {})
merge_defaults
read_content(opts)
read_post_data
rescue StandardError => e
rescue SyntaxError, StandardError, Errors::FatalException => e

This comment has been minimized.

@pathawks

pathawks Nov 5, 2017

Member

Can you explain what's going on here? Why are we rescuing FatalException?

@pathawks

pathawks Nov 5, 2017

Member

Can you explain what's going on here? Why are we rescuing FatalException?

This comment has been minimized.

@ashmaroli

ashmaroli Nov 6, 2017

Member

Because :handle_read_error is:

private
def handle_read_error(error)
  if error.is_a? SyntaxError
    Jekyll.logger.error "Error:", "YAML Exception reading #{path}: #{error.message}"
  else
    Jekyll.logger.error "Error:", "could not read file #{path}: #{error.message}"
  end

  if site.config["strict_front_matter"] || error.is_a?(Jekyll::Errors::FatalException)
    raise error
  end
end

My understanding is that unless Errors::FatalException is rescued here, the variable e is not going be a subclass of our Errors::FatalException

What say, @parkr ?

@ashmaroli

ashmaroli Nov 6, 2017

Member

Because :handle_read_error is:

private
def handle_read_error(error)
  if error.is_a? SyntaxError
    Jekyll.logger.error "Error:", "YAML Exception reading #{path}: #{error.message}"
  else
    Jekyll.logger.error "Error:", "could not read file #{path}: #{error.message}"
  end

  if site.config["strict_front_matter"] || error.is_a?(Jekyll::Errors::FatalException)
    raise error
  end
end

My understanding is that unless Errors::FatalException is rescued here, the variable e is not going be a subclass of our Errors::FatalException

What say, @parkr ?

This comment has been minimized.

@pathawks

pathawks Nov 6, 2017

Member

That doesn't make any sense. We rescue FatalException only so that we can rethrow it?

@pathawks

pathawks Nov 6, 2017

Member

That doesn't make any sense. We rescue FatalException only so that we can rethrow it?

This comment has been minimized.

@pathawks

pathawks Nov 6, 2017

Member

Oh, because we need to print the error message about not reading the file. Gotcha 👌

@pathawks

pathawks Nov 6, 2017

Member

Oh, because we need to print the error message about not reading the file. Gotcha 👌

This comment has been minimized.

@ashmaroli

ashmaroli Nov 6, 2017

Member

Right.. and Errors::FatalException is only a subclass of Ruby's RuntimeError (which is the default for a bare raise), and not a true fatal which is documented as "impossible to rescue"

@ashmaroli

ashmaroli Nov 6, 2017

Member

Right.. and Errors::FatalException is only a subclass of Ruby's RuntimeError (which is the default for a bare raise), and not a true fatal which is documented as "impossible to rescue"

This comment has been minimized.

@ashmaroli

ashmaroli Nov 6, 2017

Member

@pathawks this change is not actually required for this PR.. do you want me to remove this change so the tests can be patched faster?

@ashmaroli

ashmaroli Nov 6, 2017

Member

@pathawks this change is not actually required for this PR.. do you want me to remove this change so the tests can be patched faster?

This comment has been minimized.

@jmhooper

jmhooper Nov 12, 2017

Contributor

Since SyntaxError and Jekyll::Errors::FatalException are subclasses of StandardError, rescuing from StandardError should collect them as well.

For example, in this code:

class ASDFError < StandardError; end

def raise_error
  raise ASDFError
end

begin
  raise_error
rescue StandardError => e
  if e.is_a? ASDFError
    puts 'ASDF Error occured'
  else
    puts "Another error occured"
  end
end

...rescuing from StandardError still collects the ASDFError, and logs that an ASDFError occurs.

@jmhooper

jmhooper Nov 12, 2017

Contributor

Since SyntaxError and Jekyll::Errors::FatalException are subclasses of StandardError, rescuing from StandardError should collect them as well.

For example, in this code:

class ASDFError < StandardError; end

def raise_error
  raise ASDFError
end

begin
  raise_error
rescue StandardError => e
  if e.is_a? ASDFError
    puts 'ASDF Error occured'
  else
    puts "Another error occured"
  end
end

...rescuing from StandardError still collects the ASDFError, and logs that an ASDFError occurs.

@ashmaroli ashmaroli requested a review from parkr Nov 6, 2017

@pathawks

This comment has been minimized.

Show comment
Hide comment
@pathawks

pathawks Nov 6, 2017

Member

@jekyllbot: merge +dev

Member

pathawks commented Nov 6, 2017

@jekyllbot: merge +dev

@jekyllbot jekyllbot merged commit 0205fb9 into jekyll:master Nov 6, 2017

2 checks passed

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

@ashmaroli ashmaroli deleted the ashmaroli:jruby-bug branch Nov 6, 2017

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Nov 6, 2017

Member

This changes “production” code so I’d call it a bug fix if anything. I’m concerned as to why the ::SyntaxError is caught (a syntax error in ruby comes to mind) instead of a ::Psych::SyntaxError but I trust y’all :)

Member

parkr commented Nov 6, 2017

This changes “production” code so I’d call it a bug fix if anything. I’m concerned as to why the ::SyntaxError is caught (a syntax error in ruby comes to mind) instead of a ::Psych::SyntaxError but I trust y’all :)

@ashmaroli

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Nov 6, 2017

Member

I’m concerned as to why the ::SyntaxError is caught (a syntax error in ruby comes to mind) instead of a ::Psych::SyntaxError

You're rightly concerned. There was already a PR proposing to rescue from ::Psych::SyntaxError. I thought since he brought the matter to light first, he should get a fair chance to contribute.

Member

ashmaroli commented Nov 6, 2017

I’m concerned as to why the ::SyntaxError is caught (a syntax error in ruby comes to mind) instead of a ::Psych::SyntaxError

You're rightly concerned. There was already a PR proposing to rescue from ::Psych::SyntaxError. I thought since he brought the matter to light first, he should get a fair chance to contribute.

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