search

joomla / coding-standards

Joomla Coding Standards Definition
https://developer.joomla.org/coding-standards/basic-guidelines.html
GNU General Public License v2.0
128 stars 129 forks source link

Fix issues with closures. Fixes #230 and #231 #242

Closed wilsonge closed 5 years ago

wilsonge commented 5 years ago

So basically for example in the conditions https://github.com/joomla/coding-standards/blob/4c1503f1ae304f4a83f012751730b1e03ecfdbc2/Joomla/Tests/ControlStructures/ControlStructuresBracketsUnitTest.inc#L86 here attachRule isn't considered a condition and therefore doesn't count towards the number of levels. Instead it's considered a nested_parenthesis (whatever that means) and the same for the usort function in the other unit test we had. We blindly therefore added one level to the count but instead we should actually check for the nested_parenthesis and count how many of them there are

laoneo commented 5 years ago

How can this being tested?

wilsonge commented 5 years ago

Well you can look at the unit tests. And I manually edited my composer file locally on the currently failing framework dB package

You can’t really test the cms right now (too many other failures), but you can add your example as a test case here and see if that fails (it doesn’t)

wilsonge commented 5 years ago

Also updated my comments at the top. The reasoning for why this works magically appeared to me in a dream last night (yeah i know.....)

mbabker commented 5 years ago

The reasoning for why this works magically appeared to me in a dream last night (yeah i know.....)

Reason 329 why I'm glad I don't dream 🤣

photodude commented 5 years ago

All I can say is Brilliant. I sadly don't have time to contribute right now but wanted to stop by and say how brilliant it is you solved this issue.

[insert cartoon where coder goes to sleep and the coder's spirit gets up to keep coding]

I have an old PR over in PHPCS https://github.com/squizlabs/PHP_CodeSniffer/pull/1847 for adding this specific rule to the PHPCS core. This was one issue holding up that PR, along with needing to rewrite the unit tests with fake code rather than copied code that previously was failing the test.

photodude commented 5 years ago

PS, housekeeping item to merge this up to the 3.x dev branch for J4 use