pugjs / pug-error

Standard error objects for pug
MIT License
1 stars 3 forks source link

Machine-readable errors #1

Open TimothyGu opened 9 years ago

TimothyGu commented 9 years ago

From @robbyoconnor (jadejs/jade#2012):

Currently there are zero tools for syntax checking for neither vim nor emacs. I suspect the reason why is this lovely output:

/home/rob/.nvm/v0.10.35/lib/node_modules/jade/lib/runtime.js:231
  throw err;
        ^
Error: test.jade:2
    1|    doctype html
  > 2|  html
    3|   head
    4|     title Jade Examples
    5|     body

unexpected token "indent"
    at Parser.parseExpr (/home/rob/.nvm/v0.10.35/lib/node_modules/jade/lib/parser.js:252:15)
    at Parser.parse (/home/rob/.nvm/v0.10.35/lib/node_modules/jade/lib/parser.js:122:25)
    at parse (/home/rob/.nvm/v0.10.35/lib/node_modules/jade/lib/index.js:104:21)
    at Object.exports.compile (/home/rob/.nvm/v0.10.35/lib/node_modules/jade/lib/index.js:205:16)
    at renderFile (/home/rob/.nvm/v0.10.35/lib/node_modules/jade/bin/jade.js:227:71)
    at Array.forEach (native)
    at Object.<anonymous> (/home/rob/.nvm/v0.10.35/lib/node_modules/jade/bin/jade.js:132:11)
    at Module._compile (module.js:456:26)
    at Object.Module._extensions..js (module.js:474:10)
    at Module.load (module.js:356:32)

There is information that isn't relevant here: I don't need a stack trace for code that I don't even own.

What you could do:

Error: unexpected token "indent" in test.jade:2 
    1|    doctype html
  > 2|  html
    3|   head
    4|     title Jade Examples
    5|     body

The current error messages are nice for humans, ugly for computers to parse. The way I proposed is actually nicer for humans AND computers alike! Be even better if you just didn't include the 5 lines at all, and then I can parse this with no issues.

TimothyGu commented 9 years ago

I don't need a stack trace for code that I don't even own.

Keep in mind that stack traces are not only for users, but for us developers as well. Hence I am against the wholesale removal of everything you (or a general user) consider extraneous.

How about:

/home/rob/.nvm/v0.10.35/lib/node_modules/jade/lib/runtime.js:231
  throw err;
        ^
Error: unexpected token "indent" in test.jade:2 
    1|    doctype html
  > 2|  html
    3|   head
    4|     title Jade Examples
    5|     body

    at Parser.parseExpr (/home/rob/.nvm/v0.10.35/lib/node_modules/jade/lib/parser.js:252:15)
    at Parser.parse (/home/rob/.nvm/v0.10.35/lib/node_modules/jade/lib/parser.js:122:25)
    at parse (/home/rob/.nvm/v0.10.35/lib/node_modules/jade/lib/index.js:104:21)
    at Object.exports.compile (/home/rob/.nvm/v0.10.35/lib/node_modules/jade/lib/index.js:205:16)
    at renderFile (/home/rob/.nvm/v0.10.35/lib/node_modules/jade/bin/jade.js:227:71)
    at Array.forEach (native)
    at Object.<anonymous> (/home/rob/.nvm/v0.10.35/lib/node_modules/jade/bin/jade.js:132:11)
    at Module._compile (module.js:456:26)
    at Object.Module._extensions..js (module.js:474:10)
    at Module.load (module.js:356:32)
robbyoconnor commented 9 years ago

@TimothyGu -- Put the error and everything that's necessary for syntax checking at the top AND THEN put the stack trace -- Simple regex to parse this: Error: (.*) in (\S+):(\d+) -- and the corresponding proof it works -- compare that to the current regex necessary to parse the errors -- which was needed to extract all the necessary information -- the regex becomes WAY simpler.

Syntax checkers care about:

Error: unexpected token "indent" in test.jade:2 
    1|    doctype html
  > 2|  html
    3|   head
    4|     title Jade Examples
    5|     body
/home/rob/.nvm/v0.10.35/lib/node_modules/jade/lib/runtime.js:231
  throw err;
        ^   
 at Parser.parseExpr (/home/rob/.nvm/v0.10.35/lib/node_modules/jade/lib/parser.js:252:15)
    at Parser.parse (/home/rob/.nvm/v0.10.35/lib/node_modules/jade/lib/parser.js:122:25)
    at parse (/home/rob/.nvm/v0.10.35/lib/node_modules/jade/lib/index.js:104:21)
    at Object.exports.compile (/home/rob/.nvm/v0.10.35/lib/node_modules/jade/lib/index.js:205:16)
    at renderFile (/home/rob/.nvm/v0.10.35/lib/node_modules/jade/bin/jade.js:227:71)
    at Array.forEach (native)
    at Object.<anonymous> (/home/rob/.nvm/v0.10.35/lib/node_modules/jade/bin/jade.js:132:11)
    at Module._compile (module.js:456:26)
    at Object.Module._extensions..js (module.js:474:10)
    at Module.load (module.js:356:32)

Regex allows for completely ignoring the rest...but I shouldn't have to write a complex regex to parse this...

This is easier to parse. Let me know when this happens -- I need to update the regex for the flycheck syntax checker -- it's going to break things :smile_cat:

ForbesLindesay commented 9 years ago

Is there a reason why you can't use the properties of the error object? These would be much more robust than using the text output of the CLI.

From the error object you can get structured data for

robbyoconnor commented 9 years ago

Because i am parsing the OUTPUT -- I repeat: there are next no syntax checkers for jade. I was writing the flycheck checker for jade and was immediately like "what the hell, this is a mess" -- I am not parsing this from javascript, it's actually written in emacs lisp. Read the actual content of my comments please, and the rest of the thread as well...go google and find syntax checkers for jade, there aren't any...there's one that they claim is a linter...it's a syntax checker....and that's about it. That compiler error output is probably why...

robbyoconnor commented 9 years ago

@TimothyGu , @ForbesLindesay: Another way you could go: do what slim does:

$ slimrb slim-error.slim 

Slim::Parser::SyntaxError: Unexpected indentation 
slim-error.slim, Line 2, Column 1 
html 
^ 

Use --trace for backtrace.

If you are deadset on including the stacktrace for developers -- for generic compilation, it's not necessary...make it easy for people who are trying to parse the output and add a switch to get the stacktrace if you guys need it to debug a compiler issue...otherwise it's really not necessary.

ForbesLindesay commented 9 years ago

I strongly believe there are no separate syntax-checkers out there because nobody has felt a strong enough need for one. I attribute that at least in part to the fact that the error messages on the console are already very readable.

As for expecting me to research what flytrap is, you need to understand that you are asking me to do work to make your job easier. I'm not getting paid for this work, and hundreds of other people also want work done, so if I can't understand your request, I will ignore it.

I guess the language separation is why you're using the CLI and not the JavaScript API? Jade will always prioritise human readability over machine readers. What might be an option would be adding a --json-errors option to the CLI that could then catch the errors and log them as JSON. That would be up to @TimothyGu as he maintains the CLI.

robbyoconnor commented 9 years ago
  1. I apologize for coming off demanding.
  2. s/flytrap/flycheck -- not to offend :smile_cat:
  3. Adding a simple option to omit the stack trace would be enough -- I could live with @TimothyGu's suggestion -- it would simplify the regexp -- in fact the regexp for both my earlier suggestion and @TimothyGu's are the same -- both seem to match what I need :) -- the only suggestion would be a switch to hide the stack trace -- but I would be happy with just having the error moved up like @TimothyGu suggested:

    /home/rob/.nvm/v0.10.35/lib/node_modules/jade/lib/runtime.js:231
    throw err;
         ^
    Error: unexpected token "indent" in test.jade:2 
     1|    doctype html
    > 2|  html
     3|   head
     4|     title Jade Examples
     5|     body
    
     at Parser.parseExpr (/home/rob/.nvm/v0.10.35/lib/node_modules/jade/lib/parser.js:252:15)
     at Parser.parse (/home/rob/.nvm/v0.10.35/lib/node_modules/jade/lib/parser.js:122:25)
     at parse (/home/rob/.nvm/v0.10.35/lib/node_modules/jade/lib/index.js:104:21)
     at Object.exports.compile (/home/rob/.nvm/v0.10.35/lib/node_modules/jade/lib/index.js:205:16)
     at renderFile (/home/rob/.nvm/v0.10.35/lib/node_modules/jade/bin/jade.js:227:71)
     at Array.forEach (native)
     at Object.<anonymous> (/home/rob/.nvm/v0.10.35/lib/node_modules/jade/bin/jade.js:132:11)
     at Module._compile (module.js:456:26)
     at Object.Module._extensions..js (module.js:474:10)
     at Module.load (module.js:356:32)

    Assuming this happens and can be done -- I'm happy -- as it stands -- the flycheck checker is written and I'd just need to tweak the regexp...I just felt the output was a bit verbose...

TimothyGu commented 9 years ago

@ForbesLindesay, I'm happy to implement what I proposed. Do you think this has to wait for 2.0.0 which uses jade-error?

robbyoconnor commented 9 years ago

@TimothyGu :+1: :tada: