Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Auto-add fullstop to @throws if rule is used #3455

Closed

Conversation

pascalheidmann
Copy link

Automatic fix for cases like

/**
 * @throws FoobarException throws
*/

will be

/**
 * @throws FoobarException throws.
*/

Copy link
Contributor

@jrfnl jrfnl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have no opinion on the merit of this PR, just left some comments to get it into a mergable state in case the PR would be considered as viable.

@@ -45,7 +45,7 @@ public function getErrorList($testFile='')
switch ($testFile) {
case 'ExecutableFileUnitTest.2.inc':
case 'ExecutableFileUnitTest.4.inc':
return [1 => 1];
return [1 => 0];
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure what you're trying to do here, but this change should be undone as this is the test file for an unrelated sniff.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not really sure what changed either but without this change this unit test fails...

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How did you run the tests to come to that conclusion ?
The last CI build passed without problems, so unless you changed something either in the test files or in the sniff, there should be no reason why the tests would fail.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI - this particular test is skipped in a few cases, could it be that the way you are running the tests falls under the "skip" conditions, but isn't identified correctly ? In that case, fixing the condition would be a good thing, but that should still be done in a separate PR as it would constitute a different decision point than the decision for whether or not to accept this PR.

protected function shouldSkipTest()
{
// PEAR doesn't preserve the executable flag, so skip
// tests when running in a PEAR install.
// Also skip on Windows which doesn't have the concept of executable files.
return ($GLOBALS['PHP_CODESNIFFER_PEAR'] || (strtoupper(substr(PHP_OS, 0, 3)) === 'WIN'));
}//end shouldSkipTest()

jrfnl pushed a commit to jrfnl/PHP_CodeSniffer that referenced this pull request Oct 21, 2021
@pascalheidmann
Copy link
Author

pascalheidmann commented Oct 23, 2021

@jrfnl comments are appreciated :)
As stated the change within the phpunit test case is needed as it otherwise fails but if you have an idea/suggestion I really welcome those too.

edit: looks like the test is broken for some time and unrelated to this PR.

@gsherwood
Copy link
Member

Thanks for the PR, but this is not the kind of auto-fix I like PHPCS to do as I can't be sure a full stop is required here. Changing comments is something I think is best left to manual code review.

@gsherwood gsherwood closed this Nov 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants