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

Rescue from Psych::SyntaxError instead of SyntaxError after parsing YAML #5828

Merged
merged 7 commits into from Nov 22, 2017

Conversation

Projects
None yet
7 participants
@jmhooper
Contributor

jmhooper commented Jan 28, 2017

SafeYAML was raising a Psych::SynaxError, but the Convertible module and Document class were rescuing from a SyntaxError. This made the conditional branch for reporting that an error was a result of problems parsing the YAML unreachable and led to some more cryptic error messages.

Error reading file /Users/jonathanhooper/Developer/jekyll-test-site/pages/error.html: (<unknown>): did not find expected key while parsing a block mapping at line 2 column 1

versus

YAML Exception reading /Users/jonathanhooper/Developer/jekyll-test-site/pages/error.html: (<unknown>): did not find expected key while parsing a block mapping at line 2 column 1

This commit tells the Document class and Convertible module to rescue from the correct error class.

Rescue from Psych::SyntaxError instead of SyntaxError after parsing YAML
`SafeYAML` was raising a [`Psych::SynaxError`](https://github.com/ruby/psych/blob/master/lib/psych/syntax_error.rb), but the `Convertible` module and `Document` class were rescuing from a `SyntaxError`. This made the conditional branch for reporting that an error was a result of problems parsing the YAML unreachable and led to some more cryptic error messages.

```shell
Error reading file /Users/jonathanhooper/Developer/jekyll-test-site/pages/error.html: (<unknown>): did not find expected key while parsing a block mapping at line 2 column 1
```

versus

```shell
YAML Exception reading /Users/jonathanhooper/Developer/jekyll-test-site/pages/error.html: (<unknown>): did not find expected key while parsing a block mapping at line 2 column 1
```

This commit tells the `Document` class and `Convertible` module to rescue from the correct error class.
@jmhooper

This comment has been minimized.

Show comment
Hide comment
@jmhooper

jmhooper Jan 28, 2017

Contributor

Note that I updated the tests for Convertible.read_yaml, but I was unable to find a matching test case for Document.read. I considered writing one, but had trouble finding where Document.read was used in the code and as such was uncertain about what a test for that would look like.

Contributor

jmhooper commented Jan 28, 2017

Note that I updated the tests for Convertible.read_yaml, but I was unable to find a matching test case for Document.read. I considered writing one, but had trouble finding where Document.read was used in the code and as such was uncertain about what a test for that would look like.

@@ -46,7 +46,7 @@ def read_yaml(base, name, opts = {})
self.content = $POSTMATCH
self.data = SafeYAML.load(Regexp.last_match(1))
end
rescue SyntaxError => e
rescue Psych::SyntaxError => e

This comment has been minimized.

@parkr

parkr Mar 31, 2017

Member

This is very old code – can you leave the old SyntaxError and just add the Psych::SyntaxError?

@parkr

parkr Mar 31, 2017

Member

This is very old code – can you leave the old SyntaxError and just add the Psych::SyntaxError?

@jmhooper

This comment has been minimized.

Show comment
Hide comment
@jmhooper

jmhooper Apr 1, 2017

Contributor

I added this change in #5832 to avoid the inevitable conflict that would arise since it touches the same code. I move that this be closed if the change is accepted there.

Contributor

jmhooper commented Apr 1, 2017

I added this change in #5832 to avoid the inevitable conflict that would arise since it touches the same code. I move that this be closed if the change is accepted there.

@parkr

This comment has been minimized.

Show comment
Hide comment
@parkr

parkr Apr 3, 2017

Member

Hey @jmhooper! We try to keep pull requests small and focused, so I'm going to have to ask you to make the necessary changes here so this change can be handled separately from your other change. This will ensure that this PR has a prominent listing in our changelog so our users can know what's up when it's shipped. Sorry about the conflict between this and that PR; we can always manually merge if it's too much of an inconvenience for you. Does that sound ok?

Member

parkr commented Apr 3, 2017

Hey @jmhooper! We try to keep pull requests small and focused, so I'm going to have to ask you to make the necessary changes here so this change can be handled separately from your other change. This will ensure that this PR has a prominent listing in our changelog so our users can know what's up when it's shipped. Sorry about the conflict between this and that PR; we can always manually merge if it's too much of an inconvenience for you. Does that sound ok?

@jmhooper

This comment has been minimized.

Show comment
Hide comment
@jmhooper

jmhooper Apr 3, 2017

Contributor

Yep, totally understand! I amended the commit on the other branch.

Contributor

jmhooper commented Apr 3, 2017

Yep, totally understand! I amended the commit on the other branch.

@ashmaroli

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Nov 6, 2017

Member

@jmhooper please resolve the conflicts if you wish to further this PR. Thanks.

Member

ashmaroli commented Nov 6, 2017

@jmhooper please resolve the conflicts if you wish to further this PR. Thanks.

@parkr

parkr approved these changes Nov 7, 2017

@ashmaroli

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Nov 7, 2017

Member
lib/jekyll/document.rb:269:9: W: Lint/ShadowedException: Do not shadow rescued Exceptions.
        rescue Psych::SyntaxError, StandardError, Errors::FatalException => e
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

is most probably because both Psych::SyntaxError and Errors::FatalException are subclasses of RuntimeError. So, in theory, removing Errors::FatalException should appease RuboCop..

EDIT: Or just remove the StandardError class here.

Member

ashmaroli commented Nov 7, 2017

lib/jekyll/document.rb:269:9: W: Lint/ShadowedException: Do not shadow rescued Exceptions.
        rescue Psych::SyntaxError, StandardError, Errors::FatalException => e
        ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

is most probably because both Psych::SyntaxError and Errors::FatalException are subclasses of RuntimeError. So, in theory, removing Errors::FatalException should appease RuboCop..

EDIT: Or just remove the StandardError class here.

@ashmaroli

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Nov 12, 2017

Member

@jmhooper Requesting a minor modification on this branch.. and this is good to go.. 😃

  1. git pull origin jmhooper-psych-syntax-error to update your local branch
  2. Remove StandardError Errors::FatalException in ln#269 of document.rb
  3. Commit and push to remote.
Member

ashmaroli commented Nov 12, 2017

@jmhooper Requesting a minor modification on this branch.. and this is good to go.. 😃

  1. git pull origin jmhooper-psych-syntax-error to update your local branch
  2. Remove StandardError Errors::FatalException in ln#269 of document.rb
  3. Commit and push to remote.

DirtyF and others added some commits Nov 12, 2017

@jmhooper

This comment has been minimized.

Show comment
Hide comment
@jmhooper

jmhooper Nov 12, 2017

Contributor

@ashmaroli: Done (expect for it looks like I misspelled rescue 🙃). Thanks ya'll for picking this up

Contributor

jmhooper commented Nov 12, 2017

@ashmaroli: Done (expect for it looks like I misspelled rescue 🙃). Thanks ya'll for picking this up

@ashmaroli

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Nov 12, 2017

Member

@jmhooper Sorry for the misinformation, StandardError needs to be rescued to rescue ArgumentError as well.. So lets just disable Rubocop here temporarily

@@ -254,25 +254,27 @@
     # Read in the file and assign the content and data based on the file contents.
     # Merge the frontmatter of the file with the frontmatter default
     # values
     #
     # Returns nothing.
+    # rubocop:disable Lint/ShadowedException
     def read(opts = {})
       Jekyll.logger.debug "Reading:", relative_path
 
       if yaml_file?
         @data = SafeYAML.load_file(path)
       else
         begin
           merge_defaults
           read_content(opts)
           read_post_data
-        rescue Psych::SyntaxError, Errors::FatalException => e
+        rescue Psych::SyntaxError, Errors::FatalException, StandardError => e
           handle_read_error(e)
         end
       end
     end
+    # rubocop:enable Lint/ShadowedException
Member

ashmaroli commented Nov 12, 2017

@jmhooper Sorry for the misinformation, StandardError needs to be rescued to rescue ArgumentError as well.. So lets just disable Rubocop here temporarily

@@ -254,25 +254,27 @@
     # Read in the file and assign the content and data based on the file contents.
     # Merge the frontmatter of the file with the frontmatter default
     # values
     #
     # Returns nothing.
+    # rubocop:disable Lint/ShadowedException
     def read(opts = {})
       Jekyll.logger.debug "Reading:", relative_path
 
       if yaml_file?
         @data = SafeYAML.load_file(path)
       else
         begin
           merge_defaults
           read_content(opts)
           read_post_data
-        rescue Psych::SyntaxError, Errors::FatalException => e
+        rescue Psych::SyntaxError, Errors::FatalException, StandardError => e
           handle_read_error(e)
         end
       end
     end
+    # rubocop:enable Lint/ShadowedException
@jmhooper

This comment has been minimized.

Show comment
Hide comment
@jmhooper

jmhooper Nov 12, 2017

Contributor

@ashmaroli: Quick question. Since Psych::SyntaxError and Jekyll::Errors::FatalException are subclasses of StandardError, wouldn't it make more sense to just rescue from StandardError and let handle_read_error sort out what is a subclass of what? I.e.:

     def read(opts = {})
       Jekyll.logger.debug "Reading:", relative_path
 
       if yaml_file?
         @data = SafeYAML.load_file(path)
       else
         begin
           merge_defaults
           read_content(opts)
           read_post_data
-        rescue Psych::SyntaxError, Errors::FatalException => e
+        rescue StandardError => e
           handle_read_error(e)
         end
       end
     end

That seems to solve the problem without having to disable a cop.

Contributor

jmhooper commented Nov 12, 2017

@ashmaroli: Quick question. Since Psych::SyntaxError and Jekyll::Errors::FatalException are subclasses of StandardError, wouldn't it make more sense to just rescue from StandardError and let handle_read_error sort out what is a subclass of what? I.e.:

     def read(opts = {})
       Jekyll.logger.debug "Reading:", relative_path
 
       if yaml_file?
         @data = SafeYAML.load_file(path)
       else
         begin
           merge_defaults
           read_content(opts)
           read_post_data
-        rescue Psych::SyntaxError, Errors::FatalException => e
+        rescue StandardError => e
           handle_read_error(e)
         end
       end
     end

That seems to solve the problem without having to disable a cop.

@ashmaroli

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Nov 12, 2017

Member

rescue-ing only StandardError is going to fail tests which was "patched" in #6520 ..

Member

ashmaroli commented Nov 12, 2017

rescue-ing only StandardError is going to fail tests which was "patched" in #6520 ..

@ashmaroli

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Nov 12, 2017

Member

Since Psych::SyntaxError and Jekyll::Errors::FatalException are subclasses of StandardError, wouldn't it make more sense to just rescue from StandardError and let handle_read_error sort out what..

You may be correct.. Tests pass locally.
Lets start a new PR based of master.. this branch has been dirtied much..

Member

ashmaroli commented Nov 12, 2017

Since Psych::SyntaxError and Jekyll::Errors::FatalException are subclasses of StandardError, wouldn't it make more sense to just rescue from StandardError and let handle_read_error sort out what..

You may be correct.. Tests pass locally.
Lets start a new PR based of master.. this branch has been dirtied much..

@jmhooper

This comment has been minimized.

Show comment
Hide comment
@jmhooper

jmhooper Nov 12, 2017

Contributor

@ashmaroli: I went ahead and pushed the code to rescue from just StandardError so we can see what CI does.

Contributor

jmhooper commented Nov 12, 2017

@ashmaroli: I went ahead and pushed the code to rescue from just StandardError so we can see what CI does.

@jmhooper

This comment has been minimized.

Show comment
Hide comment
@jmhooper

jmhooper Nov 12, 2017

Contributor

...but happy to open another branch if that is what we want to do. If we're worried about a dirty history, we could just rebase it though.

Contributor

jmhooper commented Nov 12, 2017

...but happy to open another branch if that is what we want to do. If we're worried about a dirty history, we could just rebase it though.

@ashmaroli

This comment has been minimized.

Show comment
Hide comment
@ashmaroli

ashmaroli Nov 12, 2017

Member

If we're worried about a dirty history, we could just rebase it though.

No worries.. it'll be handled when the PR is being merged..
Thanks for your time and patience.. 😁

Member

ashmaroli commented Nov 12, 2017

If we're worried about a dirty history, we could just rebase it though.

No worries.. it'll be handled when the PR is being merged..
Thanks for your time and patience.. 😁

@ashmaroli

This comment has been minimized.

Show comment
Hide comment
@ashmaroli
Member

ashmaroli commented Nov 12, 2017

@jmhooper

This comment has been minimized.

Show comment
Hide comment
@jmhooper

jmhooper Nov 12, 2017

Contributor

@ashmaroli Looks like all the builds passed except for one of the appveyor ones that failed to install cucumber

Contributor

jmhooper commented Nov 12, 2017

@ashmaroli Looks like all the builds passed except for one of the appveyor ones that failed to install cucumber

@ashmaroli

LGTM! 😃

@ashmaroli ashmaroli added this to the v3.7.0 milestone Nov 12, 2017

@oe

oe approved these changes Nov 22, 2017

@DirtyF

This comment has been minimized.

Show comment
Hide comment
@DirtyF

DirtyF Nov 22, 2017

Member

@jekyllbot: merge +dev

Member

DirtyF commented Nov 22, 2017

@jekyllbot: merge +dev

@jekyllbot jekyllbot merged commit 1c469eb into jekyll:master Nov 22, 2017

2 of 3 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
WIP ready for review
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

DirtyF added a commit that referenced this pull request Dec 7, 2017

DirtyF added a commit that referenced this pull request Dec 7, 2017

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