search

mozilla / scanjs

[DEPRECATED] Static analysis tool for javascript code.
Other
429 stars 38 forks source link

Catch scan errors and show them alongside results #113

Closed mozfreddyb closed 10 years ago

mozfreddyb commented 10 years ago

This tiny pull requests adds error reporting (e.g. for syntax errors in JS files). It also adds progress monitoring (just because). Please take a look and review @pwnetrationguru :)

pwnetrationguru commented 10 years ago

Awesome, I really like the errors table and the throbber. I have a few comments below:

  1. So the throbber in the sidebar doesn't actually 'throb', fwiw. It definitely shows up when we start, and disappears when were done but it just sits there. I think we should fix that, OR, I think using Bootstrap components progress bars [1] would be awesome but then we need to think about where to put it. That's just a 'favorite color thing', so definitely feel free to ignore. I'd be happy with an animated throbber too!
  2. I really like the error table, but think it should be under CodeMirror instead of underneath the results table. We still need to tinker with this interface a bit (read fluid heights) so we'll probably touch it again, but I think that is a good spot for it.
  3. We should add another section ('Rules' and 'Riles') in the sidebar for 'Errors', to keep the interface consistent.

Thinking about it more, which would address a couple of the issues above, maybe we should make a new 'Errors' option to 'Input' and 'Output' in the sidebar? We could have a badge with the number of errors (which is a nice indicator for the user to see if there are errors right away) and when you click 'Errors' it simply hides everything and displays the errors table (just like 'Input' and 'Output' do now).

Looking forward to this PR, we need these things badly! :)

Cheers, Rob

[1] http://getbootstrap.com/components/#progress

pwnetrationguru commented 10 years ago

i forgot any emoticons, so here ya go: :boom: :+1: :turtle:

pauljt commented 10 years ago

Let's merge and improve later imho - I'd like to get newscanner landed before we make too many changes making it hard to merge...

mozfreddyb commented 10 years ago

Thanks for merging. I'm filing follow-up bugs.