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

A fix to make parse errors more visible #19

Closed
wants to merge 1 commit into from

Conversation

gabrielg
Copy link

Right now, a lot of parsing errors are actually silently swallowed. This fixes that. Rather than add a new exception class, I modified the existing ParseError to just pass along the entire original exception - this means anyone actually using the ParseError for anything will have to slightly modify their code.

@cowboyd
Copy link
Owner

cowboyd commented Jan 31, 2012

I have to say I agree with @abloom. I like the fact that the original ParseError had the same backtrace as the original error, this makes detecting where the error actually happened a snap. Is there any reason we can't have one change and not the other?

@gabrielg
Copy link
Author

I mean, I'm stashing away the original exception - you just have to call '.original.backtrace' now instead. The other exceptions that Less.js can generate don't always have backtraces, as such. For example:

https://github.com/cloudhead/less.js/blob/master/lib/less/parser.js#L320 - creates a new LessError
https://github.com/cloudhead/less.js/blob/master/lib/less/parser.js#L233 - has what I assume is a backtrace as 'stack', but which I believe would be empty in this case

Basically, I was trying to keep the semantics of rescuing ParseError to catch parsing errors, and not having to special case on a V8 error with the expected properties, vs. something the Less.js parser can return in that callback that behaves differently.

@bradphelan
Copy link

+1 swallowing exceptions is not ideal. This error was raised on less-rails because of it.

metaskills/less-rails#21

@kares
Copy link
Collaborator

kares commented May 16, 2013

this one's a bit obsolete esp. since therubyrhino support - depends on the JS runtime how native exception are visible

@kares kares closed this May 16, 2013
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants