Closed Bug 998718 Opened 6 years ago Closed 6 years ago

Indentation detection is wrong when there's a majority of outliers

Categories

(DevTools :: Source Editor, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 31

People

(Reporter: harth, Assigned: harth)

Details

Attachments

(1 file, 1 obsolete file)

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: nobody → fayearthur
Attachment #8409947 - Flags: review?(mihai.sucan)
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)
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 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+
(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.
(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.
Keywords: checkin-needed
Status: NEW → ASSIGNED
https://hg.mozilla.org/integration/fx-team/rev/4fc153a9d91b
Flags: in-testsuite+
Keywords: checkin-needed
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/4fc153a9d91b
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → Firefox 31
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.