amo-validator: Whitespace in code context is not preserved



8 years ago
3 years ago


(Reporter: kumar, Assigned: basta)




(1 attachment)

Created attachment 499182 [details]
addon with JS errors

- Validate the attached Add-on with JS enabled
- Take note of the JSON output

- The code context of the javascript error should include whitespace so the indentation can be rendered

- whitespace that existed in original code was stripped


    "context": ["if (null == this._extensions) {", "this._extensions = FireFM.Application.extensions;", "}"],
    "description": ["A dangerous or banned global object was accessed by some JavaScript code.", "Accessed object: extensions"],
    "column": 25,
    "line": 372,
    "file": "resources/fmPlayerInitializer.js",
    "tier": 4,
    "message": "Dangerous Global Object",
    "type": "error",
    "id": ["testcases_javascript_traverser", "_build_global", "dangerous_global"],
    "uid": "17951f1de15d4532be1e79a61df59b01"
Assignee: nobody → mbasta
Summary: Whitespace in code context is not preserved → amo-validator: Whitespace in code context is not preserved

Comment 1

8 years ago
The whitespace is specifically being stripped because in some (rather frequent) cases, it pushes the lines over the character threshold. It is also unclear as to how trimmed lines should be rendered with regard to whitespace. For instance:

if(...) {
   // This line contains the problem
   // This is another line

The context for this would end up looking something like:

   // This line contains the problem
   // This is another line

...which doesn't make a whole lot of sense. Also, in heavily indented code and code that's heavily indented with tab characters, the amount of indentation can exceed in some cases the potential width of the box that displays the code itself.

I'd be happy to re-include the whitespace, but I want to get confirmation on how the above issues are dealt with before I do so.

Comment 2

8 years ago
Note in my previous comment, the really long line wrapped; it is actually indented by eight characters within the while() loop. Didn't realize bugzilla would cut me off. *shakes fist menacingly at monitor*
so the validator somewhere has a character limit?  Could that maybe trigger a validation error?  It doesn't seem right to validate truncated code because that code is not true to the original addon.

As for displaying the whitespace, the UI can do trimming so I think it's better to pass in all whitespace.  Otherwise there is no reliable way to guess the indentation and it will not be as helpful to developers.

Comment 4

8 years ago
No, clouserw had me put a line limit on the context output. There's no limit for what's validated (and some developers minify their code...ugh), but when it comes time to generate a context from the file data, I think it's something like 140 characters that the lines are limited to. The idea is to only show the parts of the context that are relevant: a minified file where all the code is on a few lines isn't all that useful if you can't see the 200 characters where the problem actually exists.

For now, I'll add some logic that limits the line length but doesn't count the whitespace. We can evaluate its effectiveness from there.

Comment 5

8 years ago
All leading whitespace on a line is now preserved, though the existing truncation rules still apply (and don't count the whitespace):

Also, I've made the length of each context 3, instead of 1-3. If a particular line isn't present (i.e.: the error is on the first line), rather than eliminating the first item in the context's list, the first element is simply null (None in python). This removes any ambiguities as to which line contains the error.

Let me know if this solves the problem.
without truncation, minified files are really ugly when they have an error in them.  It sounds like you did great here.
Last Resolved: 8 years ago
Resolution: --- → FIXED
yep that's great, thanks
Product: → Graveyard
You need to log in before you can comment on or make changes to this bug.