Closed
Bug 620823
Opened 14 years ago
Closed 14 years ago
amo-validator: Whitespace in code context is not preserved
Categories
(addons.mozilla.org Graveyard :: Developer Pages, defect)
addons.mozilla.org Graveyard
Developer Pages
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: kumar, Assigned: basta)
Details
Attachments
(1 file)
412.60 KB,
application/x-xpinstall
|
Details |
STR:
- Validate the attached Add-on with JS enabled
- Take note of the JSON output
Expected:
- The code context of the javascript error should include whitespace so the indentation can be rendered
Actual:
- whitespace that existed in original code was stripped
Example:
{
"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"
}
Reporter | ||
Updated•14 years ago
|
Assignee: nobody → mbasta
Summary: Whitespace in code context is not preserved → amo-validator: Whitespace in code context is not preserved
Assignee | ||
Comment 1•14 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(...) {
while(...)
this_is_a_really_long_line_that_exceeds_the_character_limit_for_the_validator();
// This line contains the problem
// This is another line
}
The context for this would end up looking something like:
...for_the_validator();
// 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.
Assignee | ||
Comment 2•14 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*
Reporter | ||
Comment 3•14 years ago
|
||
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.
Assignee | ||
Comment 4•14 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.
Assignee | ||
Comment 5•14 years ago
|
||
All leading whitespace on a line is now preserved, though the existing truncation rules still apply (and don't count the whitespace):
https://github.com/mattbasta/amo-validator/commit/cb0b8ed1bee476073a704b34a3933aabf029b386
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.
Comment 6•14 years ago
|
||
without truncation, minified files are really ugly when they have an error in them. It sounds like you did great here.
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Reporter | ||
Comment 7•14 years ago
|
||
yep that's great, thanks
Updated•9 years ago
|
Product: addons.mozilla.org → addons.mozilla.org Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•