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

Do not swallow all exceptions on render #5495

Merged
merged 1 commit into from Oct 20, 2016

Conversation

Projects
None yet
5 participants
@pathawks
Member

pathawks commented Oct 19, 2016

Fixes #5104

@pathawks

This comment has been minimized.

Show comment
Hide comment
@pathawks
Member

pathawks commented Oct 19, 2016

@pathawks

This comment has been minimized.

Show comment
Hide comment
@pathawks

pathawks Oct 19, 2016

Member

Digging into this a bit deeper, I can trace this all the way back to commit number two, when this was still called autoblog 7dfe32a

Here is what the function looked like:

    def read_layouts
      base = File.join(self.root, "_layouts")
      dir = Dir.new(base)
      dir.each do |f|
        unless %w{. ..}.include?(f)
          name = f.split(".")[0..-2].join(".")
          self.layouts[name] = File.read(File.join(base, f))
        end
      end
    rescue Errno::ENOENT => e
      # ignore missing layout dir
    end

So, it looks like this was not ever meant to swallow some exception in a called function, it was doing all the business right there. I guess we've just carried the cruft along and now here we are, eight years later.

Member

pathawks commented Oct 19, 2016

Digging into this a bit deeper, I can trace this all the way back to commit number two, when this was still called autoblog 7dfe32a

Here is what the function looked like:

    def read_layouts
      base = File.join(self.root, "_layouts")
      dir = Dir.new(base)
      dir.each do |f|
        unless %w{. ..}.include?(f)
          name = f.split(".")[0..-2].join(".")
          self.layouts[name] = File.read(File.join(base, f))
        end
      end
    rescue Errno::ENOENT => e
      # ignore missing layout dir
    end

So, it looks like this was not ever meant to swallow some exception in a called function, it was doing all the business right there. I guess we've just carried the cruft along and now here we are, eight years later.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Oct 19, 2016

Member

Probably from the File.read call. I'd be interested in just doing a File.exist? call there instead.

Member

parkr commented Oct 19, 2016

Probably from the File.read call. I'd be interested in just doing a File.exist? call there instead.

@pathawks

This comment has been minimized.

Show comment
Hide comment
@pathawks

pathawks Oct 19, 2016

Member

Probably from the File.read call. I'd be interested in just doing a File.exist? call there instead.

Sorry to confuse; the code that I posted there is way old and gone. Was just trying to show why this rescue was originally added. We currently don't have any File.read calls 👍

Member

pathawks commented Oct 19, 2016

Probably from the File.read call. I'd be interested in just doing a File.exist? call there instead.

Sorry to confuse; the code that I posted there is way old and gone. Was just trying to show why this rescue was originally added. We currently don't have any File.read calls 👍

@parkr

parkr approved these changes Oct 19, 2016

@pathawks Merge as +bug when CI passes. 😄

@pathawks

This comment has been minimized.

Show comment
Hide comment
@pathawks

pathawks Oct 20, 2016

Member

when CI passes

You mean appveyor? master is failing on appveyor

Member

pathawks commented Oct 20, 2016

when CI passes

You mean appveyor? master is failing on appveyor

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Oct 20, 2016

Member

@pathawks Nope! Travis was failing earlier. I kicked it and it passed. 😄

@jekyllbot: merge +bug

Member

parkr commented Oct 20, 2016

@pathawks Nope! Travis was failing earlier. I kicked it and it passed. 😄

@jekyllbot: merge +bug

@jekyllbot jekyllbot merged commit eca7f46 into master Oct 20, 2016

1 of 2 checks passed

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

@jekyllbot jekyllbot added bug fix labels Oct 20, 2016

@jekyllbot jekyllbot deleted the pr/layouts-error branch Oct 20, 2016

jekyllbot added a commit that referenced this pull request Oct 20, 2016

@envygeeks envygeeks self-assigned this Oct 22, 2016

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