Add some OutputProcessor resilience #148
Conversation
If exceptions occured while formatting code, OutputProcessor#format would return `nil`. Now it should return a string instead so it can be used safely.
|
How would we get into some of the bad states shown in the tests? Bad input code? Bugs in brakeman code manipulating the AST? Will brakeman eventually output something other than the empty string? Seems like a debug statement would be helpful? Just a little uncomfortable putting something like this in unless this is truly just a short term patch and you have other plans. |
|
In this case, when I removed the The empty Sexp stuff is weird and I still don't understand where it comes from. I agree that this will probably hide some issues, which I'm not thrilled about. Even with the debug output (which there is if an exception is raised), no one will notice unless they happen to be running in debug mode. I would really like this to report an error that bubbles up to the report, but that would mean adding an error to the report while we are generating the report :P Maybe it should output a regular notification (to stdout)? More likely to be noticed then. Or do you mean instead of an empty string it should say "Processing Error" or something? That makes sense, too. |
|
Either or, someone won't be happy either way we go.
I guess I like 1) better, but I'm a power user who understands the "why". Question is, to normal users, would seeing an error in the report impact their experience negatively to a point where they won't use the tool? My guess is no. |
|
I like (1) better, too. It should be rare enough (hopefully never?) not to turn people off. |
instead of just an empty string. This should happen almost never, but hopefully this will prompt people to report bugs if it does happen instead of hiding them.
|
Word 👍 |
While this is kind of putting a bandaid on issues like #53, Brakeman should recover properly from errors while formatting code (especially since the error is already being rescued...)
Instead of
OutputProcessor#formatreturningnilwhen an error occurs or aSexpis empty, it will now return an empty string. That way when the result of the call is used (e.g. in warnings) it will be safe to expect a string.Also, tests!
😹
(While no one is probably wondering,
OutputProcessor#process_callwill likely be removed very soon in another pull request).