Closed (fixed)
Project:
Coder
Version:
8.x-2.x-dev
Component:
Coder Sniffer
Priority:
Normal
Category:
Feature request
Assigned:
Unassigned
Reporter:
Created:
20 Mar 2014 at 18:04 UTC
Updated:
28 Nov 2016 at 11:14 UTC
Jump to comment: Most recent
Comments
Comment #1
psynaptic commentedComment #2
klausiThis is difficult because we allow alignment of assignment lines, see https://drupal.org/coding-standards#functcall
So we would need an advanced sniff that checks if the next line and the line before also have an assignment and how that is indented. I think there is something similar already in the Squiz standard, maybe we could copy parts from there.
Comment #4
klausiPushed a new sniff.
Comment #5
pfrenssenOh no, I'm afraid this work has been in vain because this coding standard changed recently in #2687941: [Policy, no patch] Within the Function Calls section, delete explicit mention of padding spacing in a block of related assignments. I wasn't aware of this issue or I could have closed it before you put all the effort in fixing it :(
Beforehand we had two conflicting sections in the coding standard: one that said that assignment operators should be preceded by "a space", and another section that specifically allowed multiple spaces for alignment purposes.
The section about aligning the operators has now been removed, and as mentioned in the issue in comment #2687941-36: [Policy, no patch] Within the Function Calls section, delete explicit mention of padding spacing in a block of related assignments, "a space" can currently be interpreted as "one or more spaces":
A followup issue has been opened to further discuss and clarify this: #2816445: Forbid using more than one space around operators. Depending on the outcome of that issue the wording will be updated to "should have ONE space" or "should have at least one space" so there is no more confusion possible.
But at this very moment, developers are free to put as many spaces in front of their operators as they like, and are free to use any kind of alignment they like, they could even make christmas trees out of them if they so desire.
Comment #6
klausiHehe, I took care of all the different allowed standards right now, so the sniff should be fine :)
Please test!
Comment #7
pfrenssenI didn't test it myself but I saw from the code that this:
... now gets autofixed to this:
... but according to the current coding standards any spacing before the assignment operator is now allowed, so this case doesn't need to be flagged as an error, and it doesn't need autofixing.
The part about the aligning has been removed from the coding standards, so having any code that will autofix them to be aligned is not going to be necessary any more.
Depending on the outcome of #2816445: Forbid using more than one space around operators we will either have one single space (no alignment allowed), or any random number of spaces allowed (alignment allowed but not enforced).
I have the feeling that #2816445: Forbid using more than one space around operators might take a very long time to reach consensus, or it might not even be reached at all since people tend to be very polarized about their personal spacing preferences. We noticed this as well in the previous issue which is why we settled on allowing people to space however they want for the time being.
Edit: anyway, personally I think this change will always result in cleaner code so I'm OK for leaving it like this for the time being, at least until #2816445 is concluded.
The spirit of the coding standards is to let people choose single spacing or aligning for their personal projects and not impose any limitation on that. That's why we allow the freedom for now. I think practically everyone will be fine with how it works now. If someone might have a good argument for a 'christmas tree style' operator alignment, they can still open a new issue.
Comment #8
klausiArbitrary spacing before the alignment operator does not make sense. Either you use 1 space or you align our assignments.
In case people use weird spacing it defaults to fix it to aligned assignments right now.