Add new sniff for checking Control structure brackets are on a new line by photodude · Pull Request #1847 · squizlabs/PHP_CodeSniffer

photodude · 2018-01-08T18:08:12Z

Add new sniff for checking Control structure brackets are on a new line
Add new test for the new sniff checking Control structure brackets are on a new line

photodude · 2018-01-08T18:17:38Z

@gsherwood What Code standard should I use to format things here? I seem to have chosen the wrong one for the formatting.

jrfnl · 2018-01-08T18:28:28Z

@photodude PHPCS has its own ruleset you can use: https://github.com/squizlabs/PHP_CodeSniffer/blob/master/phpcs.xml.dist 😄

photodude · 2018-01-09T00:09:26Z

Thanks @jrfnl, hopefully I implemented the fixer right since I just ran it directly from the github files so I didn't have to mess with my PHPCS 2.9 install.

jrfnl · 2018-01-09T00:18:39Z

@photodude Want me to have a look/review ?

photodude · 2018-01-09T00:52:40Z

Do you mean to review the sniff? Yes, please.
There are probably better was to do this sniff. The sniff was originally based off the PEAR.Classes.ClassDeclaration Sniff but modified to work with control structure tokens.

I think the code style is fixed, if I can get the one comment section to stop yelling at me that it wants more or less indenting, or things at the end.

photodude · 2018-01-09T01:13:02Z

code style should be all in the green now

photodude · 2018-01-09T03:42:19Z

@jrfnl @gsherwood This is now good to go for review consideration

src/Standards/Generic/Sniffs/ControlStructures/ControlStructuresBracketsNewLineSniff.php

src/Standards/Generic/Tests/ControlStructures/ControlStructuresBracketsUnitTest.inc

src/Standards/Generic/Tests/ControlStructures/ControlStructuresBracketsUnitTest.php

gsherwood · 2018-03-19T03:40:16Z

src/Standards/Generic/Tests/ControlStructures/ControlStructuresBracketsUnitTest.inc

@@ -0,0 +1,170 @@
+<?php


The test file doesn't have any examples of comments being placed between the closing parenthesis and the opening brace of the control structures, so it's not obvious how those would be fixed. A few examples of using // comment and the phpcs annotations like // phpcs:ignore -- for reasons would be good, just to make sure the sniff copes with them on other people's code.

I'll see about adding those cases

photodude · 2018-10-02T23:40:36Z

Quick question,
I'm getting a CS error on "Implicit true comparisons prohibited; use === TRUE instead"
the error occurs for this comparison

if (array_key_exists('nested_parenthesis', $tokens[$stackPtr])) {

What is the correct solution for this?

gsherwood · 2018-10-02T23:42:37Z

@photodude

if (array_key_exists('nested_parenthesis', $tokens[$stackPtr]) === true) {

photodude · 2018-10-02T23:47:35Z

Thanks @gsherwood (and @jrfnl I saw your comment too for a moment)

I believe the last items here are

convert the unit test into fake code
add unit test cases using // comment and the phpcs annotations like // phpcs:ignore -- for reasons

src/Standards/Generic/Sniffs/ControlStructures/ControlStructuresBracketsNewLineSniff.php

gsherwood · 2018-12-10T02:53:13Z

I've done another review of this but it's not ready to merge yet. It still needs those last two items completed (remove real code from tests + add tests for comment syntax) plus:

I think this would be worth renaming to BraceNewLine to match other sniffs
Please replace tabs with spaces in the unit tests files unless the tab is testing something

I've also put a few minor changes into a review for you.

gsherwood · 2019-05-22T23:45:58Z

I've done another review of this and it's not ready to merge:

The unit test files don't match the sniff name, so they aren't actually running. The files need to be renamed so they are tested.
If the files are renamed, the unit tests will fail because the list of line numbers to error counts is wrong.
The sniff enforces indentation using tabs, which is not something other sniffs do. If the sniff wants to check indentation of the opening brace, it needs to do so using spaces. If you use tabs in your coding standard, the DisallowSpaceIndent sniff will go ahead and fix that for you, understanding where the proper tab stops are.
The unit test files is indented using tabs, so this needs to be replaced with spaces.
There is no need for an indent property in this sniff if you are just trying to align the opening brace under the IF statement. PHPCS provides 'column' indexes in the tokens array so you know where to align things. This will make the sniff possible to use for any indent type and length.

I'm going to remove this from the 3.5.0 release and there looks to be too much more to do here to get it ready.

pimjansen · 2019-07-09T08:44:28Z

@photodude any way that this will be finished? If you need some assistance let me know

photodude · 2019-07-09T15:32:47Z

@pimjansen I would love to get this finished. Assistance would be greatly appreciated and would greatly speed along the process. (I, unfortunately, have reduced availability to work on side projects like this at the moment)

fix issue with closure combined with a chained function call
fix issue with closure combined with switch case control structure
The sniff enforces indentation using tabs, which is not something other sniffs do. If the sniff wants to check indentation of the opening brace, it needs to do so using spaces. If you use tabs in your coding standard, the DisallowSpaceIndent sniff will go ahead and fix that for you, understanding where the proper tab stops are.
There is no need for an indent property in this sniff if you are just trying to align the opening brace under the IF statement. PHPCS provides 'column' indexes in the tokens array so you know where to align things. This will make the sniff possible to use for any indent type and length.
The unit test files are indented using tabs, replace tabs with spaces unless the tab is testing something (add comments to specify if the tabs are testing something specific)
The test file doesn't have any examples of comments being placed between the closing parenthesis and the opening brace of the control structures, so it's not obvious how those would be fixed. A few examples of using // comment and the phpcs annotations like // phpcs:ignore -- for reasons would be good, just to make sure the sniff copes with them on other people's code.

pimjansen · 2019-09-23T11:52:24Z

What is the easiest way to help here since this is open for way too long. What is still open and the fastest way to continue on this branch?

@photodude @gsherwood

photodude · 2019-09-28T17:25:31Z

What is the easiest way to help here since this is open for way too long. What is still open and the fastest way to continue on this branch?

@pimjansen If you can work on any of the open items in the to do checklist that would help complete this sniff.

photodude · 2020-03-21T15:26:25Z

@pimjansen I don't think I'm going to get back to working on this, as I'm now working in a different field. If you would like to take over and finish this PR all the necessary information on what needs to be completed is here.

- Sniff Error Case Changes in error sentences - Sniff errors don't end with full stops, remove periods - Replace `PHP_CodeSniffer_Tokens` with `Tokens` Co-Authored-By: photodude <photodude@users.noreply.github.com>

Leave test code as code structure but not "real" code. The intent is to only test the structure.

We don't want "real" code in the test just code structure as we are only testing code structure

…ketsNewLineUnitTest.inc

…esBracketsNewLineUnitTest.inc.fixed

…ketsNewLineUnitTest.php

return an empty array as getWarningList() must return an array

First round of correcting the line numbers for the Error list

Round 2 of correcting the line numbers for the Error list Expected errors should now be correct... LINE 225 is reporting expected 1 error but found 0 errors. but there is an indent error as the line should be indented more to match the control structure. Seems like line 225 is thinking it belongs to the closure function and not the control structure.

I believe this sniff's scope is restricted to checking the first bracket for new line and associated alignment.

…tsNewLineSniff.php

…eUnitTest.inc

…NewLineUnitTest.inc.fixed

…cketsNewLineUnitTest.php

The unit test files are indented using tabs, replace tabs with spaces unless the tab is testing something (add comments to specify if the tabs are testing something specific)

Examples of comments being placed between the closing parenthesis () and the opening brace {} of the control structures using // comment

Add examples using phpcs annotations to make sure the sniff copes with them on other people's code.

This reverts commit a94ff2f.

Add examples using phpcs annotations to make sure the sniff copes with them on other people's code.

photodude · 2023-01-29T20:47:38Z

Closing as Joomla4 is moving to PSR12. There is work to make this PR happen and I'm currently not working in PHP and don't know when I would be able to work on this again.
Sadly others Who indicated they were willing to finish this PR did not submit anything for finishing this PR.
Overall PR is now stale and should be closed.

photodude mentioned this pull request Jan 9, 2018

Fix closure indent issue and add tests joomla/coding-standards#218

Merged

gsherwood requested changes Mar 19, 2018

View reviewed changes

photodude mentioned this pull request Oct 2, 2018

Fix issues with closures. Fixes #230 and #231 joomla/coding-standards#242

Merged

gsherwood added this to the 3.4.0 milestone Nov 7, 2018

gsherwood removed this from the 3.4.0 milestone Dec 10, 2018

gsherwood requested changes Dec 10, 2018

View reviewed changes

gsherwood added this to the 3.5.0 milestone Jan 24, 2019

gsherwood mentioned this pull request May 20, 2019

Added Allman style for ControlStructures #2509

Closed

peternolland self-assigned this May 22, 2019

gsherwood unassigned peternolland May 22, 2019

gsherwood removed this from the 3.5.0 milestone May 22, 2019

photodude mentioned this pull request Jul 9, 2019

Add test case for issue found in the CMS with closures joomla/coding-standards#250

Merged

pimjansen mentioned this pull request Apr 16, 2020

Allman styling control structures PHPCSStandards/PHPCSExtra#51

Open

gsherwood and others added 21 commits February 27, 2022 11:41

Apply suggestions from code review

58aea1c

- Sniff Error Case Changes in error sentences - Sniff errors don't end with full stops, remove periods - Replace `PHP_CodeSniffer_Tokens` with `Tokens` Co-Authored-By: photodude <photodude@users.noreply.github.com>

reduce code to not be "real" code.

e0ebd96

Leave test code as code structure but not "real" code. The intent is to only test the structure.

Update fixed test to reflect code structure for testing

cec373a

We don't want "real" code in the test just code structure as we are only testing code structure

change variable to dummy var

28af909

Rename ControlStructuresBracketsUnitTest.inc to ControlStructuresBrac…

d9a5fe7

…ketsNewLineUnitTest.inc

Rename ControlStructuresBracketsUnitTest.inc.fixed to ControlStructur…

fe7a667

…esBracketsNewLineUnitTest.inc.fixed

Rename ControlStructuresBracketsUnitTest.php to ControlStructuresBrac…

0a3b3f2

…ketsNewLineUnitTest.php

Add case: Closure + chained function call

9fcdfe0

Add case: Closure + chained function call

091c732

Add case: Closure + chained function call

a9f3953

getWarningList() must return an array

f0d7b69

return an empty array as getWarningList() must return an array

Correct Error list line numbers

dfe3c58

First round of correcting the line numbers for the Error list

This Sniff only checks the first bracket is new line and alignment

658cd27

Sniff only checks the first bracket for new line and alignment.

df946e0

I believe this sniff's scope is restricted to checking the first bracket for new line and associated alignment.

Update and rename ControlStructuresBracketsNewLineSniff.php to Bracke…

7109af5

…tsNewLineSniff.php

Rename ControlStructuresBracketsNewLineUnitTest.inc to BracketsNewLin…

abab9f8

…eUnitTest.inc

Rename ControlStructuresBracketsNewLineUnitTest.inc.fixed to Brackets…

8af01c5

…NewLineUnitTest.inc.fixed

Update and rename ControlStructuresBracketsNewLineUnitTest.php to Bra…

69a4463

…cketsNewLineUnitTest.php

Fix code style violations

241fdbd

Use Spaces in the unit tests

e11b5d7

The unit test files are indented using tabs, replace tabs with spaces unless the tab is testing something (add comments to specify if the tabs are testing something specific)

photodude force-pushed the ControlBracketsNewLine branch from e1849ea to e11b5d7 Compare February 27, 2022 18:58

photodude added 5 commits February 27, 2022 12:13

Match Errors and Fixed Errors

8d79e50

Add comment between ) and {

0fbec03

Examples of comments being placed between the closing parenthesis () and the opening brace {} of the control structures using // comment

Examples with phpcs annotations

a94ff2f

Add examples using phpcs annotations to make sure the sniff copes with them on other people's code.

Revert "Examples with phpcs annotations"

13c1d50

This reverts commit a94ff2f.

Add examples using phpcs annotations

0af21b6

Add examples using phpcs annotations to make sure the sniff copes with them on other people's code.

photodude closed this Jan 29, 2023

photodude deleted the ControlBracketsNewLine branch January 29, 2023 20:48

Conversation

photodude commented Jan 8, 2018

Uh oh!

photodude commented Jan 8, 2018

Uh oh!

jrfnl commented Jan 8, 2018

Uh oh!

photodude commented Jan 9, 2018

Uh oh!

jrfnl commented Jan 9, 2018

Uh oh!

photodude commented Jan 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

photodude commented Jan 9, 2018

Uh oh!

photodude commented Jan 9, 2018

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gsherwood Mar 19, 2018

Choose a reason for hiding this comment

Uh oh!

photodude May 23, 2018

Choose a reason for hiding this comment

Uh oh!

photodude commented Oct 2, 2018

Uh oh!

gsherwood commented Oct 2, 2018

Uh oh!

photodude commented Oct 2, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gsherwood commented Dec 10, 2018

Uh oh!

gsherwood commented May 22, 2019

Uh oh!

pimjansen commented Jul 9, 2019

Uh oh!

photodude commented Jul 9, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pimjansen commented Sep 23, 2019

Uh oh!

photodude commented Sep 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

photodude commented Mar 21, 2020

Uh oh!

photodude commented Jan 29, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

photodude commented Jan 9, 2018 •

edited

Loading

photodude commented Oct 2, 2018 •

edited

Loading

photodude commented Jul 9, 2019 •

edited

Loading

photodude commented Sep 28, 2019 •

edited

Loading

photodude commented Jan 29, 2023 •

edited

Loading