kschiess / parslet

A small PEG based parser library. See the Hacking page in the Wiki as well.
kschiess.github.com/parslet
MIT License
809 stars 95 forks source link

Rspec has issues with 'cause' on 'Parslet::ParseFailed' #151

Closed olbrich closed 7 years ago

olbrich commented 8 years ago

rspec seems to now traverse exception chains and attempts to properly format them for output. When it encounters a Parslet::ParseFailed exception it has problems because of the cause method.

In this case, rspec recursively calls cause on exceptions which works the first time to get the Parslet::Cause object, but fails after that because Parslet::Cause does not have a cause method.

Fixing this is going to either require defining a cause method on Parslet::Cause that returns nil, or renaming the cause method on Parslet::ParseFailed to something else.

In addition, because Ruby automatically populates the cause of an exception with the original exception when re-raising exceptions, it is not possible to simply bypass this problem by raising a new exception without setting the cause.

I would be inclined to suggest renaming the method since it is actually in conflict with a method on Exception that has been there since ruby 2.1.

I would be happy to create a pull request for this, but wanted to hear your thoughts on the approach since this would represent a fairly significant and possibly breaking change.

olbrich commented 8 years ago

if others encounter this problem... this works.

module Parslet
  class ParseFailed < StandardError
    alias_method :parse_failure_cause, :cause

    def cause
      nil
    end
  end
end
kschiess commented 8 years ago

You're right, the name collision is probably hurting parslet. Please propose a new name; I would also gladly receive a (breaking change) PR for this. We'll bump the version number to reflect this.

aoitaku commented 8 years ago

How about this one?

Parslet::Atoms::Base#parse_with_debug

I think the name collision should be fixed, however, it seems ok to use parse_with_debug instead of parse when testing with RSpec.

kschiess commented 8 years ago

Pray tell: what is the collision in parse_with_debug? The local variable that we use?

aoitaku commented 8 years ago

No collision in parse_with_debug. I had intended to say "the name collision" means "the collision between Parslet::ParseFailed#cause and cause called by rspec".

kschiess commented 8 years ago

Yeah, parse_with_debug in rspec tests is probably fine or even good usage.

Too bad that the PR seems to be abandoned - the issue here is a real one. Maybe you want to pick it up and produce a patch that is non-invasive?