Closed stronk7 closed 10 months ago
BTW, note that the first alternative above:
In practice, render inexistent any difference between mono-line and multi-line, because the former always has the closer in the same/unique (mono) line.
Making this default behaviour seems to make sense to me. If people want to assert that the closer of a multi-line array is on a different line, one can use another sniff like Squiz.Arrays.ArrayDeclaration.CloseBraceNewLine
.
@stronk7 Thanks for bringing this to my attention.
Changing the default behaviour would be a breaking change, so I'm not keen on that, but I'm missing another alternative solution:
MissingMultilineCloserSameLine
, FoundMultilineCloserSameLine
.Having the separate error codes would allow for excluding the applicable error code, while retaining the default behaviour as is.
The extra error code could also be considered a breaking change, but one with a smaller impact than changing the default behaviour.
What do you think ?
Making this default behaviour seems to make sense to me. If people want to assert that the closer of a multi-line array is on a different line, one can use another sniff like
Squiz.Arrays.ArrayDeclaration.CloseBraceNewLine
.
Yeah, or, also, PHPCSExtra's NormalizedArrays.Arrays.ArrayBraceSpacing available here, that is about enforcing newlines and so on (obviously we don't use that one, because we allow the cases commented above.
- Introduce new error codes for this specific situation, something along the lines of
MissingMultilineCloserSameLine
,FoundMultilineCloserSameLine
.
Aha, that's a good 5th alternative, had not thought about it! For sure it will be enough for us to be able to ignore those error codes.
- Introduce new error codes for this specific situation, something along the lines of
MissingMultilineCloserSameLine
,FoundMultilineCloserSameLine
.Aha, that's a good 5th alternative, had not thought about it! For sure it will be enough for us to be able to ignore those error codes.
In that case, I'm open to a PR to make that change.
Sup, will give a try to it soon, thanks!
Bug Description
The
NormalizedArrays.Arrays.CommaAfterLast
does its job pretty well. But there is a case that is hitting us a lot (because of our own coding standard explicitly allowing it), and that I think that can be hitting others out there too.Right now we support all these arrays as valid:
And the Sniff is working ok for all the cases but the 3rd one. And it seems logic not to require a comma there, because, if there are new items to be added to the array later, the line is going to change, yes or yes (the closer will need to be moved.
Given the following reproduction Scenario
Ensure that "Multi-multi line with same line closer " cases don't require a comma.
The issue happens when running this command:
... over a file containing this code:
I'd expect the following behaviour
No errors are reported.
Instead this happened
Five errors are reported.
Environment
Additional Context (optional)
This was originally reported for us @ https://github.com/moodlehq/moodle-cs/issues/76 And this change seems to fix the problem:
Note the TODO that I've put there. IMO there are 4 alternatives:
$validValues
, say:enforce_but_when_sameline_closer
.enforce_skipped_when_sameline_closer
.If there is any insight/preference about any of the 1-3 alternatives above... I'm happy preparing a PR with tests and so on...
Ciao :-)
Tested Against
develop
branch?develop
branch of PHPCSExtra.