dont fire possible_missing_comma if intendation is present
#5083
Conversation
clippy_lints/src/formatting.rs
Outdated
| let lo = cx.sess.source_map().lookup_char_pos(span.lo()); | ||
| lo.file | ||
| .get_line(lo.line - 1 /* line numbers in `Loc` are 1-based */) | ||
| .map(|line| line[..lo.col.0].chars().map(|c| if c == '\t' { 8 } else { 1 }).sum()) |
There was a problem hiding this comment.
I wouldn't special case tab chars here. Since tab chars are always formatted differently, adding 8 for tabs can be correct in one editor, but wrong in others.
There was a problem hiding this comment.
I know, but 8 is de-facto default tab size, and IMO it's better than not accounting for it at all (or maybe better solution is to not lint at all if tab chars are present?)
There was a problem hiding this comment.
I think we should lint, when tabs are present, since formatting something like
1
- 2,
// the same as
1
- 2,
// in some other editor
with tabs and then rely on the formatting is weird.
Also if someone uses tabs instead of spaces, they probably use it in every line, so it doesn't have to be special cased.
What I'm saying is, that the biggest argument against tabs is, that the formatting is inconsistent across editors. This is a heuristic that relies on consistent formatting. So I wouldn't try to solve the tab-formatting problem at all here.
There was a problem hiding this comment.
I special cased it because of
1, 2, 3 -
4,
5, 6
(although I don't know if that's a possible rustfmt output).
Well worst case scenario if I we don't special case it we get a false-positive, so I'll remove it.
There was a problem hiding this comment.
You can configure rustfmt to use hard_tabs. But then rustfmt will use tabs for indentation and spaces for alignment. So also in this case special casing tabs isn't necessary.
|
Thanks! @bors r+ |
|
📌 Commit a234aef has been approved by |
dont fire `possible_missing_comma` if intendation is present Closes #4399 changelog: dont fire `possible_missing_comma` if intendation is present I suspect there is already a utils function for indentation (but searching indent didn't yield a function for that), and my solution is certainly not universal, but it's probably the best we can do.
|
☀️ Test successful - checks-travis, status-appveyor |
Closes #4399
changelog: dont fire
possible_missing_commaif intendation is presentI suspect there is already a utils function for indentation (but searching indent didn't yield a function for that), and my solution is certainly not universal, but it's probably the best we can do.