Closed
Bug 998718
Opened 10 years ago
Closed 10 years ago
Indentation detection is wrong when there's a majority of outliers
Categories
(DevTools :: Source Editor, defect)
DevTools
Source Editor
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 31
People
(Reporter: harth, Assigned: harth)
Details
Attachments
(1 file, 1 obsolete file)
2.61 KB,
patch
|
msucan
:
review+
|
Details | Diff | Splinter Review |
I have one example I just ran into: ``` response.body.should have_css("p a", :text => "hello") response.body.should have_css("p a", :text => /[hH]ello(.+)/i) # True if there is a anchor tag with text matching regex ``` It should say that it couldn't determine the indentation. Instead it says the indentation is 51 spaces! It's because the only indented line is that one stray comment. We should probably just throw out anything that's over 8 spaces.
Assignee | ||
Comment 1•10 years ago
|
||
Assignee: nobody → fayearthur
Attachment #8409947 -
Flags: review?(mihai.sucan)
Assignee | ||
Comment 2•10 years ago
|
||
Comment on attachment 8409947 [details] [diff] [review] Throw out > 8 space indents in detection Review of attachment 8409947 [details] [diff] [review]: ----------------------------------------------------------------- actually, gonna add a test.
Attachment #8409947 -
Flags: review?(mihai.sucan)
Assignee | ||
Comment 3•10 years ago
|
||
This'll both not recognize indents that are >8 chars, and also throw out an indent if it only occurs once, as it might be a fluke.
Attachment #8409947 -
Attachment is obsolete: true
Attachment #8410574 -
Flags: review?(mihai.sucan)
Comment 4•10 years ago
|
||
Comment on attachment 8410574 [details] [diff] [review] Throw out >8 space indents and indents with less than two occurances Review of attachment 8410574 [details] [diff] [review]: ----------------------------------------------------------------- This is fine but when I read the bug report I was thinking: - we could throw out comment lines. - we could throw out lines with indents way above average, so if there's a line which goes far too high it's clearly not part of normal indentation rules of the file. - 8 chars seems like a hardcoded arbitrary limit. r+ because we dont need too much effort in this kind of code. ;)
Attachment #8410574 -
Flags: review?(mihai.sucan) → review+
Comment 5•10 years ago
|
||
(In reply to Mihai Sucan [:msucan] from comment #4) > r+ because we dont need too much effort in this kind of code. ;) ... and by this I mean we need only to cater to the common cases - obviously indentation detection is a very welcome feature for all the different files we work with.
Assignee | ||
Comment 6•10 years ago
|
||
(In reply to Mihai Sucan [:msucan] from comment #4) > - we could throw out comment lines. > - we could throw out lines with indents way above average, so if there's a > line which goes far too high it's clearly not part of normal indentation > rules of the file. > - 8 chars seems like a hardcoded arbitrary limit. All good thoughts. As for comments, it's nice that it's language agnostic right now.
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 8•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/4fc153a9d91b
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 31
Updated•6 years ago
|
Product: Firefox → DevTools
You need to log in
before you can comment on or make changes to this bug.
Description
•