Split the errors, adding the *CloserSameLine ones

[stronk7]

Some standards may want to have different rules when the array closer is in the same line than the last element.

When that happens, the new *CloserSameLine errors are emitted, allowing to disable them via ruleset.

For testing, a couple of cases have been added to CommaAfterLastUnitTest.1.inc and then, the fixtures have been 100% duplicated into CommaAfterLastUnitTest.3.inc that runs with the *CloserSameLine errors disabled.

A simple diff shows the 8 cases in which the outcome is different, as expected.

Fixes #283.

[stronk7]

OT, sharing because it happened to me... I just found that something in composer is not installing things properly, neither vendor/bin/phpunit or composer test (and others) seem to be working by default. Had to remove the lock file, remove the platform constraint and other custom installations to get it working locally.

[jrfnl]

@stronk7 Thanks for the PR, I'll try to review it over the next few days.

OT, sharing because it happened to me... I just found that something in composer is not installing things properly, neither vendor/bin/phpunit or composer test (and others) seem to be working by default. Had to remove the lock file, remove the platform constraint and other custom installations to get it working locally.

Just tried myself and all seems fine, so I'm not sure what happened for you. Removing the lock file and (re-)running composer update (on PHP 7.4 or lower) is never a bad idea though if you cloned a while back. There is no committed composer.lock file, so that is normal.

As for the "platform constraint": the repo doesn't have one, so I don't know what you mean.

The only real (known) problem with the tests is that you need to run them locally on PHP 7.4 or lower as otherwise you run into issues with PHPUnit 7.5. There are work-arounds for that, see: https://github.com/PHPCSStandards/PHPCSExtra/blob/develop/.github/workflows/quicktest.yml , but the simple solution is to just run on PHP 7.4. This is a limitation in the PHPCS native test suite which is used for the tests and I'm trying to get that limitation lifted.

[jrfnl]

@stronk7 I've just had a quick glance at the PR and first thing I noticed is that the new error code is not just reserved for multi-line arrays. This will need to be fixed, as differentiating the error code doesn't make sense for a single line array where the closer will always be on the same line and it would basically mean the FoundSingleLine and MissingSingleLine codes would be replaced with the new codes which is a much bigger and unnecessary breaking change.

[jrfnl]

P.S.: no need to worry about the code coverage builds failing - this is a known issue and I'm working with Coveralls to fix that.

[stronk7]

Thanks for the composer comments and tricks… I’ll try again tomorrow from scratch. Maybe I had some old lock file around and that caused the troubles…

Regarding applying it only to multilines, cannot but agree. As commented in the issue, it really doesn’t make sense for singles. I will amend the PR tomorrow, thanks!

[stronk7]

Patch updated to only apply the "CloserSameLine" error code suffix to multi lines (with tests amended to ensure that's what it's being done).

Ciao :-)

[jrfnl]

P.S.: if you rebase the PR on the current develop, the code coverage builds should start working ;-)

[stronk7]

Sorry to be a pain. Please let me know if you have questions.

No worries, I'm used, LOL :-)

Thanks for the review, I'll go towards all the the test changes and suggestions later today.

[stronk7]

Ok, I think I've followed all your suggestions and the new candidate PR follows all them.

Only point that I'm not 100% convinced is the one about not covering with tests what happens when the CloserSameLine error is disabled. While I agree that it's phpcs itself the one to make the disabling to work... it's the only way we have to ensure that we are setting the CloserSameLine new error properly. I mean, current tests don't cover the new code introduced in the Sniff. At all (in fact they will pass exactly the same without the Sniff changes).

Note I don't mind much, because for sure we are going to add some tests covering that in our standard, but I think that it should be tested here because, as said, it's not covering the new real code at all.

Ciao :-)

[stronk7]

Adding on my previous comment... what I'd add to the tests is this (I think it's the minimum patch I've been able to create):

https://github.com/stronk7/PHPCSExtra/commit/f971d5b0e74f44daff633119ebd2b87fa85f4618

You can see that it doesn't change the tests outcome (all the new lines are ok, because we are ignoring the CloserSameLineerror) but, that way, we are getting covered the new Sniff code.

For your consideration, I'm happy to fixup this PR with that commit or to leave it apart (and to add a similar testing to our standard, ensuring that the new error codes work as expected).

Ciao :-)

[jrfnl]

@stronk7

Only point that I'm not 100% convinced is the one about not covering with tests what happens when the CloserSameLine error is disabled. While I agree that it's phpcs itself the one to make the disabling to work... it's the only way we have to ensure that we are setting the CloserSameLine new error properly. I mean, current tests don't cover the new code introduced in the Sniff. At all (in fact they will pass exactly the same without the Sniff changes).

Note I don't mind much, because for sure we are going to add some tests covering that in our standard, but I think that it should be tested here because, as said, it's not covering the new real code at all.

Adding on my previous comment... what I'd add to the tests is this (I think it's the minimum patch I've been able to create):

https://github.com/stronk7/PHPCSExtra/commit/f971d5b0e74f44daff633119ebd2b87fa85f4618

You can see that it doesn't change the tests outcome (all the new lines are ok, because we are ignoring the CloserSameLineerror) but, that way, we are getting covered the new Sniff code.

Okay, so let's split this up:

1. The actual code does get hit via these tests (also see the code coverage), so the code does actually get tested.
2. The tests wouldn't fail without this change.

I agree that 2 is problematic, and that's inherent to the PHPCS native test framework, which is being used as a base for these tests. It's on my never-ending to-do list to create an alternative test setup for external standards, which would allow for testing that the error codes are as expected (and more). Also see: https://github.com/PHPCSStandards/PHPCSUtils/issues/72

I'd be okay with you adding the extra tests you propose. I think only one for each type (missing/found) would be fine and that those can be in the same // phpcs:set... blocks as the tests you've got in place now.

[stronk7]

I'd be okay with you adding the extra tests you propose. I think only one for each type (missing/found) would be fine and that those can be in the same // phpcs:set... blocks as the tests you've got in place now.

Ok, will amend the commit to incorporate ONE missing and ONE found case to the 2 "new sections" fixtures introduced here.

OT: Since we started to use phpcs long ago... one of the first things we did is to switch to a "richer" testing alternative. It supports the normal line => num_errors thing of phpcs and more, with both the error message and the error code being available for substring assertions. Just sharing in case it gives some idea to you (in fact I think we shared about it @ phpcs ages ago).

[jrfnl]

I'd be okay with you adding the extra tests you propose. I think only one for each type (missing/found) would be fine and that those can be in the same // phpcs:set... blocks as the tests you've got in place now.

Ok, will amend the commit to incorporate ONE missing and ONE found case to the 2 "new sections" fixtures introduced here.

👍🏻

OT: Since we started to use phpcs long ago... one of the first things we did is to switch to a "richer" testing alternative. It supports the normal line => num_errors thing of phpcs and more, with both the error message and the error code being available for substring assertions. Just sharing in case it gives some idea to you (in fact I think we shared about it @ phpcs ages ago).

Looks interesting, but not quite what I had in mind. We use a custom test case in PHPCompatibility as well.

Basically, what I'm looking to create would be a "best of these worlds" situation, where it would give sniff writers the flexibility to test what they want to test, while also making the test suites straight-forward to maintain. Keep en eye on PHPCSUtils or subscribe to the issue if you'd like to stay informed.

[stronk7]

Patch amended, by adding the 2 agreed new cases (using phpcs:ignore).

Keep en eye on PHPCSUtils or subscribe to the issue if you'd like to stay informed. 👍