Join GitHub today
GitHub is home to over 28 million developers working together to host and review code, manage projects, and build software together.Sign up
Bug fix: DisallowSpaceIndent did not recognize nor fix mixed tab/space indentations. #1404
I noticed two bugs in the
This PR fixes that.
referenced this pull request
Apr 2, 2017
This behaviour is intentional. Some indentation can only be achieved by using spaces after the normal tab indent, so this sniff does not assume that everything is perfectly aligned to a tab stop. The scope indent sniff makes the same assumption, which is why it never checks for exact indentation unless you tell it to. And when you do, it almost always reports false positives.
DisallowTabIndents is much easier because a tab is always banned due to it having no use in a space-based indent rule.
Changing the behaviour of this sniff would be a BC break, as would changing the definitions of the metric.
To make it a better sniff, I think it needs to continue to allow the space exception, but probably only if those spaces indent the code to a non tab stop. So if a tab could be used instead, the spaces would throw an error and be replaced. But if putting a tab there would cause the code to begin at a different column, the spaces should not be reported or replaced. That's the job of the ScopeIndent sniff.
Hi @gsherwood, thanks for the feedback.
Looking at the code, IMHO the existing behaviour of the sniff is inconsistent with the above statement.
When a line starts with a space and is not a file/class level docblock and there are less spaces than the
Similarly, when the line starts with a tab, but also has spaces, no replacement is ever done, even when there are sets of 4 (
Studying the code in even more detail, I find more inconsistencies as the replacement seems to be ordered wrong.
// Original line: .......->...// code <- 7 spaces, 1 tab, 3 spaces // Would be replaced by: ->->..->//code <- 2 tabs, 2 spaces, 1 tab // While I would have expected: ->->->..//code <- 3 tabs, 2 spaces
What I would like to suggest is to align the behaviour of the sniff with your statement and with what I understand to be the intention of the sniff: Disallowing spaces for alignment, but allowing precision alignment using spaces.
In practice this would mean:
I would not consider that a BC break, but a bug fix as the sniff currently does not behave in line with the intention of the sniff.
If you agree with that, the only question is what to do with the current behavioral inconsistency: should a set of less than 4 spaces in a line beginning with a space still be replaced with a tab ? (when not
As for the metrics, am I correct that any precision spacing should be ignored, but other than that, the metrics should be recorded ?
Or should the
I've made the necessary code changes for this in line with the above. If you could clarify your stance on the points I've described here, I can finish it off and push a replacement commit.
There seems to be a mix of misunderstanding and inconsistent testing (I cannot reproduce your results). The sniff currently reports on leading spaces in indentation, and replaces those leading spaces with tabs. In my testing, it is still working like this.
For example, you're
But this doesn't really matter because you came to the same conclusion as me, although wrote it up more succinctly:
I complexly agree with this. It is what I was suggesting in my final comment, which would make the sniff more useful.
As for your questions:
If we assume the rule is "take the indentation and rewrite it to use tabs" than the answer would be that if the indentation is less than a tab width, it would remain as spaces. If it is greater, as many spaces will be removed as possible. The
I think the rules are:
Ok, verifying my implementation with your comments now.
This would change the behaviour for the unit tests on line 18 and 19 where the indentation would no longer be fixed. Is that ok ?
Edit: Also line 22 and 24 though the whitespace order for those lines would be changed.
One more question: in the existing implementation, file/class docblocks are ignored for the metrics.
Yeah, I think it is ok for lines 18,19,22,24 to have different test results now. Especially lines 18 and 19, which were changing the indent level of the code.
Yes, they need to be ignored or else tab-indented code would end up with space-indented metrics for all top-level docblocks, like file and class comments.
Hi @gsherwood, thanks for those responses.
I've replaced the original commit with a new one which takes everything we've discussed into account.
The logic of the original sniff has essentially been rewritten to allow for this.
Details for the fixed sniff:
Please let me know if you need any further changes.