Skip to content
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

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

Closed
olbrich opened this issue Jan 8, 2016 · 6 comments
Closed

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

olbrich opened this issue Jan 8, 2016 · 6 comments

Comments

@olbrich
Copy link
Contributor

olbrich commented Jan 8, 2016

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
Copy link
Contributor Author

olbrich commented Jan 19, 2016

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
Copy link
Owner

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.

johnnaegle added a commit to johnnaegle/parslet that referenced this issue May 10, 2016
…iled'. rename cause to parse_failure_cause
@aoitaku
Copy link
Contributor

aoitaku commented Jun 10, 2016

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
Copy link
Owner

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

@aoitaku
Copy link
Contributor

aoitaku commented Jun 30, 2016

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
Copy link
Owner

kschiess commented Jul 1, 2016

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?

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

No branches or pull requests

3 participants